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 sequence / integration tests #220

Merged
merged 18 commits into from
Jan 13, 2025
Merged

Conversation

schuenke
Copy link
Collaborator

This is still work in progress. More tests will be added.

@btasdelen btasdelen mentioned this pull request Jan 9, 2025
@FrankZijlstra
Copy link
Collaborator

I've attempted to add the example sequence tests (and some bugfixes to make the tests pass) to this PR. I also reworked the tests into a TestSequence class that avoids recreating the sequence multiple times (important for the more CPU-intensive sequence examples).

The tests run into an issue where it can't find the examples folder... Locally this works fine by adding the parent directory to sys.path, but on github this fails. Any thoughts @schuenke ? An alternative would be to move the sequence examples back into the main package...

Besides this:

  • I think the Approx class should be taken out of test_sequence.py, it is a utility that could be of use in other tests (it extends pytest.approx to also work recursively on lists, dicts and SimpleNamespaces).
  • I believe we should not ignore all warnings in the tests. For example, one of the sequence examples had timing errors, that now get checked by write_seq and give a warning. I'm not sure I understand the reason to give a UserWarning for the default duration of make_block_pulse. But it's easily avoided by specifying a duration.

@schuenke
Copy link
Collaborator Author

The tests run into an issue where it can't find the examples folder... Locally this works fine by adding the parent directory to sys.path, but on github this fails. Any thoughts @schuenke ? An alternative would be to move the sequence examples back into the main package...

The path problem should be fixed. Now it's "just" some AssertionErrors left. Maybe you can have a look again @FrankZijlstra? Reading your code comments I have the impression that you already spent some time on the write -> read issue 😄

@FrankZijlstra
Copy link
Collaborator

The path problem should be fixed. Now it's "just" some AssertionErrors left. Maybe you can have a look again @FrankZijlstra? Reading your code comments I have the impression that you already spent some time on the write -> read issue 😄

It looks like all tests are running the same sequence (with TotalDuration 0.64). I think test_func gets overwritten?

@btasdelen btasdelen mentioned this pull request Jan 10, 2025
@schuenke schuenke marked this pull request as ready for review January 11, 2025 08:26
@schuenke schuenke changed the base branch from dev to master January 13, 2025 07:25
@schuenke schuenke changed the title Add more tests before major refactoring Add sequence / integration tests Jan 13, 2025
@schuenke schuenke merged commit 33a9dda into imr-framework:master Jan 13, 2025
6 checks passed
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