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

Migrating Quicktest to PyTest and adding dockerized self hosted runner #549

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

taha-abdullah
Copy link
Contributor

  1. added tests for:

    • segmentation stats
    • image data
  2. migrated tests from unittest to pytest framework

  3. changed fastsurfer to run multiple subjects

  4. added new workflow (quicktest_runner) to run on dockerized self-hosted runner

@taha-abdullah taha-abdullah marked this pull request as draft July 19, 2024 11:02
@taha-abdullah
Copy link
Contributor Author

@dkuegler requesting review

@taha-abdullah taha-abdullah changed the title Pytest Migrating Quicktest to PyTest and adding dockerized self hosted runner Jul 24, 2024
@dkuegler dkuegler self-requested a review July 30, 2024 10:03
.github/workflows/quicktest.yaml Outdated Show resolved Hide resolved
.github/workflows/quicktest.yaml Outdated Show resolved Hide resolved
.github/workflows/quicktest.yaml Show resolved Hide resolved
.github/workflows/quicktest_runner.yaml Outdated Show resolved Hide resolved
.github/workflows/quicktest_runner.yaml Outdated Show resolved Hide resolved
test/quick_test/test_images.py Outdated Show resolved Hide resolved
test/quick_test/test_images.py Outdated Show resolved Hide resolved
test/quick_test/test_stats.py Outdated Show resolved Hide resolved
test/quick_test/test_stats.py Outdated Show resolved Hide resolved
test/quick_test/test_stats.py Outdated Show resolved Hide resolved
@taha-abdullah
Copy link
Contributor Author

@dkuegler requesting review after suggested fixes

Copy link
Member

@dkuegler dkuegler left a comment

Choose a reason for hiding this comment

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

I have not really checked test_images or test_stats yet, changes from other files should probably be applied there first too .

.github/workflows/quicktest.yaml Outdated Show resolved Hide resolved
.github/workflows/quicktest.yaml Show resolved Hide resolved
.github/workflows/quicktest.yaml Show resolved Hide resolved
.github/workflows/quicktest.yaml Outdated Show resolved Hide resolved
.github/workflows/quicktest.yaml Show resolved Hide resolved
test/quick_test/test_errors_in_logfiles.py Outdated Show resolved Hide resolved
test/quick_test/test_file_existence.py Outdated Show resolved Hide resolved
test/quick_test/test_file_existence.py Outdated Show resolved Hide resolved
test/quick_test/test_file_existence.py Outdated Show resolved Hide resolved
test/quick_test/test_file_existence.py Outdated Show resolved Hide resolved
@dkuegler
Copy link
Member

Please clean up/squash the git history a bit.

test/quick_test/conftest.py Outdated Show resolved Hide resolved
.github/workflows/quicktest_runner.yaml Outdated Show resolved Hide resolved
.github/workflows/quicktest_runner.yaml Show resolved Hide resolved
.github/workflows/quicktest_runner.yaml Outdated Show resolved Hide resolved
test/quick_test/common.py Outdated Show resolved Hide resolved
@dkuegler
Copy link
Member

Please also apply changes from the reviews and clean up the git history.

@taha-abdullah
Copy link
Contributor Author

@dkuegler please review

taha-abdullah and others added 2 commits September 10, 2024 11:23
- added support for multiple test subjects and parameterization in all tests

- reorganized file structure

- separate stats files for each aseg and aparc+DKT

- moving fixtures and common functions to common.py and conftest.py

quicktest_runner.yaml:

- removed check-output job
- added THIS_RUN_DIR to seperate different runs
- documentation fixes in quicktest.yaml

- docstring fixes in test_errors_in_logfiles.py

- removed loop over assert in test_file_existence.py
@taha-abdullah
Copy link
Contributor Author

@dkuegler please review again

… thus simplifying, and streamlining the workflow and improving execution time significantly.

Adding Labelled output directories for each run

Removing unnecessary import statements from pytest files

Cleaning up code:

- migrating from os.path.join to pathlib Path

- changing str to Path

- cleaning up comments and docstrings

- ran ruff on code to fix code style
Copy link
Member

@dkuegler dkuegler left a comment

Choose a reason for hiding this comment

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

Some minor fixes required before we merge. Generally, we need to make sure when we merge the commit, it passes the test itself.

.github/workflows/quicktest_runner.yaml Show resolved Hide resolved
test/quick_test/test_errors_in_logfiles.py Show resolved Hide resolved
test/quick_test/test_file_existence.py Outdated Show resolved Hide resolved
test/quick_test/test_file_existence.py Show resolved Hide resolved
test/quick_test/test_images.py Show resolved Hide resolved
test/quick_test/test_images.py Show resolved Hide resolved
test/quick_test/test_images.py Show resolved Hide resolved
test/quick_test/test_stats.py Show resolved Hide resolved
test/quick_test/test_stats.py Outdated Show resolved Hide resolved
test/quick_test/test_stats.py Show resolved Hide resolved
- ruff fixes

- docstrings: added description for fixture filled arguments
@dkuegler dkuegler merged commit ef3843c into Deep-MI:dev Sep 18, 2024
6 checks passed
@dkuegler dkuegler deleted the pytest branch September 18, 2024 13:44
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.

2 participants