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

Add test for sct_dmri_display_bvecs, then add -v argument to make the test pass #3387

Merged
merged 4 commits into from May 12, 2021

Conversation

joshuacwnewton
Copy link
Member

Checklist

GitHub

PR contents

Description

#3091 added loglevel setting steps to all SCT scripts. However, sct_dmri_display_bvecs never had a verbose argument.

This was never caught because there is no test for sct_dmri_display_bvecs. So, this adds an (admittedly very basic) test for the script. (A more thorough test would require some refactoring of sct_dmri_display_bvecs.)

This test fails on master, but passes on this branch after -v is added.

Linked issues

Fixes #3386.

@joshuacwnewton joshuacwnewton added bug category: fixes an error in the code sct_dmri_display_bvecs context: labels May 12, 2021
@joshuacwnewton joshuacwnewton added this to the 5.3.1 milestone May 12, 2021
@joshuacwnewton joshuacwnewton requested a review from a team May 12, 2021 12:54
Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

thanks!

@valosekj
Copy link
Member

@joshuacwnewton Thanks for incredibly fast workaround. I have just tried your branch and now it works! Thanks!

@joshuacwnewton
Copy link
Member Author

Hrm... the Catalina build has stalled for the past 2.75hr... restarting now. 😅

@joshuacwnewton
Copy link
Member Author

OK, it's still stalling even after a restart. I ran into a similar problem in #3373 with aa23154:

@pytest.mark.skipif(sys.platform == "darwin", reason="Test hangs on macOS 10.15 CI runners (cause currently unknown)")

Since this is the second time this has happened, I'd like to try and find an explanation...

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented May 12, 2021

Ah, my hunch is that it has to do with tests that invoke matplotlib.pyplot.show(). Both scripts (sct_denoising_onlm and sct_dmri_display_bvecs) use it, and pyplot is mentioned in our Wiki as causing problems on headless systems.

EDIT: Confirmed by 3b5aefe!

@joshuacwnewton joshuacwnewton merged commit a62fb67 into master May 12, 2021
@joshuacwnewton joshuacwnewton deleted the jn/3386-fix-sct_dmri_display_bvecs branch May 12, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category: fixes an error in the code sct_dmri_display_bvecs context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sct_dmri_display_bvecs fails in v5.3.0
3 participants