fix test that relied on slow garbage collection

braingram commented 6 months ago

Changes

This PR makes a small change to TestWeldXFile.test_show_header_memory_usage to keep the opened WeldxFile in memory while the assert is computed.

Without this PR the test includes: https://github.com/BAMWelDX/weldx/blob/bb213a77ee79e8c982adfec0d56b9d5a3efee7b1/weldx/tests/asdf_tests/test_weldx_file.py#L452

Breaking apart the above line the following steps occur: 1) a WeldxFile object is instantiated using fn as the input 2) the x value is extracted from the WeldxFile 3) the contents of x are compared against large_array 4) etc...

If garbage collection is triggered between 2 and 3 above (after x is extracted but before the comparison with large_array) there is no reference keeping WeldxFile in memory (and keeping the underlying asdf file open). The garbage collector is therefore free to close the asdf file and cause the comparison to fail (because the contents of x have not yet been read since this is a lazy-loaded array).

This PR updates the test to use a with context to keep the WeldxFile open.

An example of where this test fails can be seen here: https://github.com/asdf-format/asdf/actions/runs/7390148220/job/20104458462?pr=1721 (Note that this is a CI run for changes to asdf tree traversal algorithms. I cannot say definitively but I have the suspicion that the current asdf tree algorithms produce difficult-to-collect structures. This might be resulting in objects surviving a few generations of collection. This would explain why the issue fixed in this PR is not yet causing test failures).

Checks

[ ] updated CHANGELOG.md
[ ] updated tests/
[ ] updated doc/
[ ] update example/tutorial notebooks
[ ] update manifest file

github-actions[bot] commented 6 months ago

Test Results

2 188 tests ±0 2 187 :white_check_mark: ±0 2m 3s :stopwatch: ±0s 1 suites ±0 1 :zzz: ±0 1 files ±0 0 :x: ±0

Results for commit 1ea1df23. ± Comparison against base commit 66a9ce42.

:recycle: This comment has been updated with latest results.

marscher commented 6 months ago

Good catch @braingram! Thank you! LGTM.

braingram commented 6 months ago

Thanks for giving the PR a look! Let me know if there are any changes you'd like me to make including:

Adding a changelog (since this is a minor test change I figured a changelog entry wasn't called for)
I'm not sure what's causing the docs failures but they appear unrelated (and mention ImportError: cannot import name 'Glyph' from 'matplotlib.ft2font')
there is one failing test with this PR ([doctest] weldx.asdf.file.WeldxFile) which appears to be due to slight header differences between asdf 3.0 and the expected output (one difference is the removal of BuiltinExtension in asdf 3.0)

For the last one some liberal use of ... appears to fix the issue. I'll open a separate PR with a fix for the doctest.

CagtayFabry commented 6 months ago

Thank you for the explanation @braingram

We will need some time to update everything for the 3.0 API changes

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (66a9ce4) 96.45% compared to head (1ea1df2) 96.45%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #903 +/- ## ======================================= Coverage 96.45% 96.45% ======================================= Files 95 95 Lines 6287 6287 ======================================= Hits 6064 6064 Misses 223 223 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

BAMWelDX / weldx