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

Two more system tests. #98

Merged
merged 6 commits into from
Sep 23, 2022
Merged

Two more system tests. #98

merged 6 commits into from
Sep 23, 2022

Conversation

samcunliffe
Copy link
Member

@samcunliffe samcunliffe commented Sep 20, 2022

This PR is adds two more test cases provided by @prmunro (thanks!) and another 2/4 of #30: a test w/ exi present and a test w/ exdetintegral.

The first is very fast:

py.test tdms/tests/system/test_arc10.py  73.70s user 10.40s system 563% cpu 14.916 total

The second a little slower (but not the slowest):

py.test tdms/tests/system/test_arc09.py  704.33s user 40.89s system 681% cpu 1:49.37 total
py.test tdms/tests/system/test_arc03.py  1171.14s user 66.51s system 527% cpu 3:54.44 total

And notably I had to tweak the HDF5File class to cope with groups.

@samcunliffe samcunliffe added the testing Adding or requesting more test coverage label Sep 20, 2022
Had to tweak HD5File to traverse groups.
@samcunliffe samcunliffe changed the title Another system test. Two more system tests. Sep 20, 2022
@t-young31
Copy link
Member

apologies – don't really have time for a proper review today. but in general more tests = better so generally happy. only thought is to reduce the system size so we can be more comprehensive i.e. have both 3D and 2D examples, for instance(?)

@samcunliffe
Copy link
Member Author

apologies – don't really have time for a proper review today. but in general more tests = better so generally happy. only thought is to reduce the system size so we can be more comprehensive i.e. have both 3D and 2D examples, for instance(?)

I like it. Although perhaps we merge this now (since slow protection >> no protection). And I'll ticket myself to speedup the rest of the tests.

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Putting each test in a new file is resulting in a lot of boilerplate code - it looks like you're running through the same steps each time, so I'd recommend writing one test function an parametrizing it. Something like:

@pytest.mark.parametrize('input_file, reference_file', [('arc_09/in/pstd_fs.mat', 'arc_09/out/pstd_fs.mat'), ...])
def test_tdms():
    ...

tdms/tests/system/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@t-young31 t-young31 left a comment

Choose a reason for hiding this comment

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

Looks great to me – agree some slow protection is definitely better than no fast protection. However, I'd be inclined to make the tests fast ASAP, to limit the trees we're burning 🌲 🔥

tdms/tests/system/utils.py Show resolved Hide resolved
@prmunro
Copy link
Collaborator

prmunro commented Sep 22, 2022

Hi All, I can further shorten the time for tests to complete without compromising the validity of the test. This would mean uploading new test files. Should I do this?

@samcunliffe
Copy link
Member Author

Hi All, I can further shorten the time for tests to complete without compromising the validity of the test. This would mean uploading new test files. Should I do this?

Hey @prmunro ,
Yes, that would be very helpful indeed! We should be able to update the zenodo version and keep the same links and everything. So we potentially don't even need to PR.

samcunliffe and others added 3 commits September 23, 2022 08:05
Improvements to language/clarity of developers' documentation.

Co-authored-by: David Stansby <dstansby@gmail.com>
@samcunliffe samcunliffe merged commit 8c1b4a6 into main Sep 23, 2022
@samcunliffe samcunliffe deleted the another-system-test branch October 10, 2022 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Adding or requesting more test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants