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

Fix tests after #236 and cleanup push_pr workflow #237

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

schuenke
Copy link
Collaborator

@schuenke schuenke commented Jan 17, 2025

  • set rf.dead_time of refocussing pulse in write_epi_se (was causing an error after merge of Add warning to make_block_pulse() if rf.delay is changed to rf.dead_time #236). Sorry, my fault!
  • cleaned push_pr workflow:
    • limit repository access to minimum
    • added Python 3.12 to matrix
    • included flag for sigpy tests in matrix (allows to run everything in parallel)
    • removed unnecessary use of conda env in tests
    • added dependency caching for speed up
    • added concurrency to save resources and avoid unnecessary duplication
    • added coverage report
    • ensure test job fails if Failures or Errors occur

All in all, this hopefully simplifies the testing process, removes unnecessary complexity, and is generally better structured.

@schuenke schuenke added bug-fix Fixes something enhancement New feature or request labels Jan 17, 2025
Copy link

github-actions bot commented Jan 17, 2025

Tests Skipped Failures Errors Time
1165 18 💤 0 ❌ 0 🔥 2m 8s ⏱️

Copy link

Tests Skipped Failures Errors Time
1165 18 💤 0 ❌ 0 🔥 2m 7s ⏱️

@FrankZijlstra
Copy link
Collaborator

The "slow" tests cannot run on their own, at least how it's currently set up, because it needs to load the sequence into the TestSequence object first with test_sequence, which is not marked as a "slow" test. A solution would be to change the class such that each test can create the sequence if it does not exist yet, or maybe we can mark test_sequence in a way that it always runs? (i.e. without -m, with -m "slow" and with -m "not slow"?).

@schuenke
Copy link
Collaborator Author

The "slow" tests cannot run on their own, at least how it's currently set up, because it needs to load the sequence into the TestSequence object first with test_sequence, which is not marked as a "slow" test. A solution would be to change the class such that each test can create the sequence if it does not exist yet, or maybe we can mark test_sequence in a way that it always runs? (i.e. without -m, with -m "slow" and with -m "not slow"?).

Yeah, realized that as well. I introduced a "required_for_slow" marker and then use pytest -m "slow or required_for_slow" in the sequence tests. That seems to work. Will push this in a few minutes after some final tests.

@schuenke
Copy link
Collaborator Author

okay, in the 3rd try on my local PC the slow / plot tests failed as well because of some random tkinter error. 4th run was okay, again. However, in the GitHub workflow it seems buggy. Takes ages. IMO we should just remove the current plot tests (and the slow marker) completely. It doesn't really tests anything anyway, does it?

Instead I would introduce some dedicated tests for the plot() functionality at some point.

Would you be fine with that @FrankZijlstra ? If not, I will just leave it as it was before and simply don't run the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Fixes something enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants