Skip to content

Add integration tests for mass_nifti_pic.py #1274

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

Merged
merged 17 commits into from
Apr 24, 2025

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Apr 17, 2025

This adds integration tests for the mass_nifti_pic.py script.

That exercise revealed a few error messages from mass_nifti_pic.py that were incorrect or in the wrong order so correcting them at the same time.

@cmadjar cmadjar added Blocked Merge it and you die Area: CI PR or issue related to continuous integration, including automated tests and static checks labels Apr 17, 2025
@cmadjar cmadjar self-assigned this Apr 17, 2025
@cmadjar cmadjar added this to the 27.0 milestone Apr 17, 2025
@cmadjar cmadjar requested a review from maximemulder April 17, 2025 21:15
@cmadjar cmadjar removed their assignment Apr 17, 2025
@cmadjar cmadjar removed the Blocked Merge it and you die label Apr 17, 2025
@cmadjar cmadjar modified the milestone: 27.0 Apr 17, 2025
Copy link
Contributor

@maximemulder maximemulder left a comment

Choose a reason for hiding this comment

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

The tests look good but I am not sure if we should be using hard-coded IDs in them.

If you think that's fine, then I am ok with it, if you also prefer to not use hard-coded IDs, you can query the files by path, and then use file.id in the tests.

The only hard requirement to merge the PR is rename or delete parameter_file.py, for the rest do what you like.

Copy link
Contributor

@maximemulder maximemulder left a comment

Choose a reason for hiding this comment

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

Perfect 👍

@cmadjar cmadjar merged commit 1b25af6 into aces:main Apr 24, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI PR or issue related to continuous integration, including automated tests and static checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants