Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken tests #1079

Merged
merged 3 commits into from Jul 10, 2021
Merged

Fix broken tests #1079

merged 3 commits into from Jul 10, 2021

Conversation

manc-lx
Copy link
Contributor

@manc-lx manc-lx commented Jun 24, 2021

Changes:

  • fixed group_members()
  • fixed project_roles()
  • fixed create_board()
  • fixed # tests, new tests result: 165 passed, 4 xfailed, 1 skipped
    test_dashboard.py: 1 test == 2 xfailed
    test_security_level.py: 1 test == 2 xfailed
    test_service_desk.py: 1 test skipped
    there 3 tests are expected, they are all because of the standalone jira instance from the docker image has no these data defined by default. To make them pass, we need to create dashboard or security schema, unfortunately, there is no REST API to create them.

Changes:
* fixed group_members()
* fixed project_roles()
* fixed create_board()
* fixed # tests, new tests result: 165 passed, 4 xfailed, 1 skipped
  test_dashboard.py: 2 xfailed
  test_security_level.py: 2 xfailed
  test_service_desk.py: 1 skipped
  there 5 cases are expected, they are all because of the standalone jira instance from the docker image has no these data defined by default. To make them pass, need to create dashboard or security schema, unfortunately, there is no REST API to create them.
@manc-lx
Copy link
Contributor Author

manc-lx commented Jun 24, 2021

@ssbarnea @adehad could you please review this pull request?

@ssbarnea ssbarnea added the bug label Jun 24, 2021
@ssbarnea ssbarnea requested a review from adehad June 24, 2021 11:17
@ssbarnea ssbarnea changed the title fixed tests Fix broken tests Jun 24, 2021
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was ready to give a super +++ for fixing the broken tests, once I started to look at the review I see that this changes does a considerably number of changes to the library itself, including its public APIs!

I do not know the reasons behind those changes but I know for sure that they should not be allowed to go inside as part of of a change named "fix broken tests". The impact on users would be huge.

Probably we should keep this open and gradually make PRs for things that are clear, also lowering its size once those merge.

@ssbarnea ssbarnea added the Status: Blocked This cannot or should not be merged for one reason or another. label Jun 24, 2021
Copy link
Collaborator

@adehad adehad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some excellent improvements here @manc-lx

Echoing @ssbarnea I would suggest splitting this into separate merge requests where there are changes required to the client.py/resources.py side of things.

jira/client.py Outdated Show resolved Hide resolved
jira/client.py Outdated Show resolved Hide resolved
jira/client.py Outdated Show resolved Hide resolved
jira/client.py Outdated Show resolved Hide resolved
jira/client.py Outdated Show resolved Hide resolved
tests/resources/test_dashboard.py Show resolved Hide resolved
tests/resources/test_issue.py Show resolved Hide resolved
tests/resources/test_issue.py Show resolved Hide resolved
tests/resources/test_remote_link.py Outdated Show resolved Hide resolved
tests/resources/test_remote_link.py Outdated Show resolved Hide resolved
@manc-lx
Copy link
Contributor Author

manc-lx commented Jun 25, 2021

While I was ready to give a super +++ for fixing the broken tests, once I started to look at the review I see that this changes does a considerably number of changes to the library itself, including its public APIs!

I do not know the reasons behind those changes but I know for sure that they should not be allowed to go inside as part of of a change named "fix broken tests". The impact on users would be huge.

Probably we should keep this open and gradually make PRs for things that are clear, also lowering its size once those merge.

Some of the changes to the library itself is about parameter name changes, I can certainly keep them back compatible.

While some of the changes is because the APIs are broken on JIRA Server 8.9.0 (such as create_board() requests 'filterId' instead of 'project_ids'), I'm sure they worked on some version of JIRA Server. I agree we should not let the users down, probably we need to set up tests infrastructures for various JIRA versions and run the tests against all of them, just like what you guys have did against py36/37/38/39. I'm not sure whether it's feasible to create docker images for different JIRA Server versions, does anyone has dock image experiences know about this?

Revert most of changes made in the library itself. With this change, 1 more test xfailed:
tests/resources/test_issue.py::IssueTests::test_agile, this fail is because GreenHopper returns error when creating sprint on standalone jira docker image.

Latest tests results: 164 passed, 6 xfailed, 1 skipped
@manc-lx
Copy link
Contributor Author

manc-lx commented Jun 25, 2021

@ssbarnea @adehad I resolved all your review questions, please let me know if you have more questions.

@manc-lx manc-lx requested review from ssbarnea and adehad June 25, 2021 09:20
Copy link
Collaborator

@adehad adehad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I've left a comment but that isn't for this PR, as it would involve us moving to a new (non-deprecated) endpoint, so probably beyond the "fix broken tests" side of things.

@@ -1232,7 +1232,17 @@ def group_members(self, group: str) -> OrderedDict:

result = {}
for user in r["users"]["items"]:
result[user["id"]] = {
# 'id' is likely available only in older JIRA Server, it's not available on newer JIRA Server.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, looking at the existing docs I have no idea how long since they removed id . I looked as far back as v6.0.1 for Jira Server and it still didn't have an id property for the returned users.

image

We can also see that the group endpoint is Deprecated, group/member is suggested instead (which will allow us to remove the legacy pagination in this function).

https://docs.atlassian.com/software/jira/docs/api/REST/8.13.6/#api/2/group-getUsersFromGroup
https://developer.atlassian.com/cloud/jira/platform/rest/v2/api-group-groups/#api-rest-api-2-group-get

Let's make a separate PR to deal with this, there we may remove hasId and instead have something like:

result[user.get("accountId", user.get("name"))] =

@adehad
Copy link
Collaborator

adehad commented Jun 25, 2021

I agree we should not let the users down, probably we need to set up tests infrastructures for various JIRA versions and run the tests against all of them,

I like the idea but definitely think we are a while off from reaching that point. I think the order to spend our effort might look like:

  1. Improve the test coverage
  2. Test both the Cloud and Server/Data Center (maybe this should be 1.)
  3. Test multiple versions (might only be possible on Server at the moment)

@adehad adehad added Status: Ready for Merge PR looks good to merge and removed Status: Blocked This cannot or should not be merged for one reason or another. labels Jun 25, 2021
@adehad adehad merged commit f763075 into pycontribs:master Jul 10, 2021
@manc-lx manc-lx deleted the fix_tests branch July 12, 2021 04:45
svermeulen pushed a commit to svermeulen/jira that referenced this pull request Oct 31, 2021
* fixed tests

Changes:
* fixed group_members()
* fixed project_roles()
* fixed create_board()
* fixed # tests, new tests result: 165 passed, 4 xfailed, 1 skipped
  test_dashboard.py: 2 xfailed
  test_security_level.py: 2 xfailed
  test_service_desk.py: 1 skipped
  there 5 cases are expected, they are all because of the standalone jira instance from the docker image has no these data defined by default. To make them pass, need to create dashboard or security schema, unfortunately, there is no REST API to create them.

* Fixed review issues

Revert most of changes made in the library itself. With this change, 1 more test xfailed:
tests/resources/test_issue.py::IssueTests::test_agile, this fail is because GreenHopper returns error when creating sprint on standalone jira docker image.

Latest tests results: 164 passed, 6 xfailed, 1 skipped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Status: Ready for Merge PR looks good to merge
Projects
None yet
3 participants