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 coverage on remote xdist nodes #19630

Merged
merged 1 commit into from Jan 23, 2019
Merged

Conversation

jmbowman
Copy link
Contributor

@jmbowman jmbowman commented Jan 18, 2019

Fixed coverage reporting with remote xdist workers by using the correct [paths] configuration in .coveragerc; pytest-cov tries to set it automatically, but has a bug in doing so that I just reported. Also fixed a few other things I noticed while debugging:

  • Check out the correct branch on workers for non-PR builds; this should reduce the rsync duration.
  • Automatically create the reports directory on xdist workers; the paver commands already do this, but direct pytest runs from devstack benefit from having this here.
  • Updated some assertions in a test I saw flake once, to get more useful data if it happens again.

@jmbowman
Copy link
Contributor Author

jenkins run pipeline python

1 similar comment
@jmbowman
Copy link
Contributor Author

jenkins run pipeline python

@jmbowman
Copy link
Contributor Author

jenkins run pipeline python

@jmbowman
Copy link
Contributor Author

jenkins run pipeline python

@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@@ -476,12 +476,12 @@ def test_get_user_group_id_for_partition(self):

# get a group assigned to the user
group1_id = self.partition_service.get_user_group_id_for_partition(self.user, user_partition_id)
self.assertEqual(group1_id, groups[0].id)
assert group1_id == groups[0].id
Copy link
Contributor

Choose a reason for hiding this comment

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

^ should we feel empowered to use pytest asserts instead of unittest asserts from here on out? I understand why you did it, I am just curious about it as a policy (following up after our conversation w/ dave o, in which consistency was brought up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think our current stance is "don't waste time trying to convert all the assertions, but feel free to use pytest assertions in new code or where the old assertions aren't providing enough useful information to diagnose known problems".

@jmbowman jmbowman merged commit ad037ab into master Jan 23, 2019
@jmbowman jmbowman deleted the jmbowman/fix_xdist_coverage branch January 23, 2019 19:46
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, January 24, 2019.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@jmbowman
Copy link
Contributor Author

@nedbat This will need to be cherry-picked into Ironwood in order for coverage to work in its Jenkins tests

@nedbat
Copy link
Contributor

nedbat commented Jan 28, 2019

I've cherry-picked this onto ironwood.master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants