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

Remove xdist from requirements and pytest config #3212

Merged
merged 1 commit into from Feb 11, 2021

Conversation

Drulex
Copy link
Collaborator

@Drulex Drulex commented Feb 8, 2021

Checklist

GitHub

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've assigned a reviewer

PR contents

Description

This PR removes the default usage of pytest-xdist when running the unit tests. See minutes from dev meeting:

  • Motivation: we have hit some limitations/issues with regards to concurrency issues (Session-Scoped Fixtures are not Session-Scoped with Pytest-Xdist pytest-dev/pytest-xdist#271 (comment), Fix test download concurrency bug. #3152). This complicates the ongoing effort to port CI to github actions (Port CI to Github Actions #3125, Fix test download concurrency bug. #3152)

  • Other annoyances such as xdist disabling -s (--capture=no) thus preventing the user from viewing the output of stdout which is confusing for users. (Relevant dev Wiki page about this issue.)

    • This issue specifically is causing Julien to disable xdist for local testing, for example.
  • Doesn’t add value in CI since almost all VMs are single-core.

  • Alternatives:

    • pytest-parallel (to be evaluated)
    • Disabled by default config. People who know what they are doing can enable it (or it can be enabled in ci.sh only).
    • Strategies in Speed up CI #2674, particularly split tests across jobs; but we also need to consider that each job needs to spend installation time (there is a sweet spot in terms of number of tests per job)
  • CI time on GH actions: total ~15min; on Travis: ~1hr

    • Largely dominated by waiting for the macOS VM to boot

In a nutshell: there is not a lot of value added (at the moment) in using xdist and working around the issues mentioned above. Tests can happily run sequentially and it won't make a difference for 99% of the people. People who do care about parallelization can still use xdist locally.

@Drulex Drulex added the tests context: unit, integration, or functional tests label Feb 8, 2021
@Drulex Drulex self-assigned this Feb 8, 2021
@Drulex
Copy link
Collaborator Author

Drulex commented Feb 8, 2021

TODO update wiki

@joshuacwnewton
Copy link
Member

So, by removing pytest-xdist, we wouldn't need the locking fix from #3152, right? (cc'ing @kousu)

@Drulex
Copy link
Collaborator Author

Drulex commented Feb 8, 2021

So, by removing pytest-xdist, we wouldn't need the locking fix from #3152, right? (cc'ing @kousu)

Indeed. @kousu may still contribute the fix upstream if he wants.

@joshuacwnewton joshuacwnewton merged commit 74f068d into master Feb 11, 2021
@joshuacwnewton joshuacwnewton deleted the aj/dont-use-xdist branch February 11, 2021 14:14
@joshuacwnewton joshuacwnewton changed the title Don't use xdist by default. Remove xdist from requirements and pytest config Feb 11, 2021
@joshuacwnewton
Copy link
Member

I've updated the Wiki here: https://github.com/neuropoly/spinalcordtoolbox/wiki/Contributing:-Testing-your-contribution#a-note-about-parallelization

@Drulex
Copy link
Collaborator Author

Drulex commented Feb 11, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature tests context: unit, integration, or functional tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants