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

Replace all 45 sct_testing tests with Pytest equivalents #3373

Merged
merged 66 commits into from Jul 5, 2021

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented May 5, 2021

Checklist

GitHub

PR contents

Description

This PR replaces sct_testing with pytest. The motivation for this is summarized here: #3093 (comment)

45 of the commits in this PR do the following:

  • Removes an original sct_testing test.
  • Removes a "backwards compatibility" wrapper test.
  • Adds a new pytest test to replace these older tests.

Then, there are 4 additional commits:

  • f0defbf: Adds a fixture to ensure that the tests run in the correct directory.
  • 762af7f: Adds a custom marker so that pytest -m sct_testing behaves identically to sct_testing.
  • b78ce08: Fixes a bug in sct_register_multimodal.py discovered by the new tests.
  • d06f961: Removes spinalcordtoolbox/scripts/sct_testing.py itself.

How to review this PR

This PR is quite large, and I apologize for that. But, I think it's best to do this in one go. If we try to split up the PR, we will have a half-working sct_testing if only one PR is merged. (If you have any ideas for how to make this easier to review, please let me know. 😅)

To review this PR, please compare each old sct_testing test to the new pytest test to make sure they match up. (For example, compare testing/test_sct_analyze_lesion.py with unit_testing/cli/test_cli_sct_analyze_lesion.py.)

To make this easier, use "Commit" view, because each commit will contain a pair of test files.

Note: The old sct_testing tests can be a bit confusing. So, to help explain, I have written a Wiki page called sct_testing.

Linked issues

Fixes #3093.
Fixes #2133.

@joshuacwnewton joshuacwnewton added tests context: unit, integration, or functional tests sct_testing context: refactoring category: improves code structure without affecting user-facing functionality labels May 5, 2021
@joshuacwnewton joshuacwnewton force-pushed the jn/3093-migrate-sct_testing-to-pytest branch 5 times, most recently from d06f961 to e6099d8 Compare May 5, 2021 14:24
@joshuacwnewton joshuacwnewton marked this pull request as ready for review May 5, 2021 14:53
@joshuacwnewton joshuacwnewton force-pushed the jn/3093-migrate-sct_testing-to-pytest branch 2 times, most recently from d964d7a to ee44471 Compare May 6, 2021 12:28
@joshuacwnewton
Copy link
Member Author

For transparency, I'm going to rebase the changes from 0e3434e, 4c8d34d, f6ab574, and ee44471 to make sure that the commit view is still useful for comparing the tests from this PR.

@joshuacwnewton joshuacwnewton force-pushed the jn/3093-migrate-sct_testing-to-pytest branch from ee44471 to 8e4231b Compare May 6, 2021 13:22
@joshuacwnewton joshuacwnewton force-pushed the jn/3093-migrate-sct_testing-to-pytest branch from c0eb8dd to 00a61ca Compare June 22, 2021 12:57
@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Jun 22, 2021

Thank you so, so much for taking the time to provide such detailed feedback, @kousu.

I think I've answered all of your feedback. If it's not too much trouble, could you go through your comments and mark them as "resolved" if you're satisfied by the changes? 🙂

If everything looks good, I can rebase the bugfixes into the main commits so that the "Commit" view is useful again. (Or, maybe I can just merge at this point -- there's diminishing returns to multiple rounds of reviews here, especially because we're going to continue to update these tests with more thorough checks.)

@joshuacwnewton joshuacwnewton requested a review from kousu July 5, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring category: improves code structure without affecting user-facing functionality sct_testing context: tests context: unit, integration, or functional tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace older sct_testing tests with new pytest tests Fix broken integrity test for sct_label_utils
3 participants