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

Cleanup asdf._tests._helpers #1703

Merged
merged 10 commits into from
May 7, 2024
Merged

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Dec 13, 2023

Description

This PR cleans up asdf._tests._helpers removing all but assert_tree_match. This PR removes:

  • get_test_data_path replaced with a test fixture using importlib.resources (data for commands tests were moved to _tests.data to simplify paths)
  • assert_roundtrip_tree replaced with asdf.testing.helpers.assert_roundtrip_object and more explicit assert statements to compare the feature being tested in any given test
  • yaml_to_asdf replaced with asdf.testing.helpers.yaml_to_asdf
  • get_file_sizes replaced with uses of pathlib.Path and calls to stat().st_size
  • display_warnings unused, removed
  • assert_no_warnings all uses removed, tests already run with a filter that turns warnings into errors, a pytest configuration was added to _tests/conftest to make sure that this filter applies to all runs (even through through pytest --pyargs asdf as is run during the Build CI test)
  • _assert_extension_type_correctness unused, removed

Although not removed assert_tree_match was simplified (removing unused arguments and other unused code).

assert_roundtrip_tree performed a number of tests for round tripping (that are not all performed by testing.helpers.roundtrip_object):

The simplification of assert_tree_match will further break the test_reference_files.py tests that have not been running since Feb 2022. When run manually (by checking out asdf-standard as if it were still a submodule) these fail without this PR and likely these should be updated and possibly moved to the asdf-standard test suite (which is run as part of the asdf CI). There is already an open issue for these tests: #1522

Removal of assert_roundtrip_tree allowed for the removal of the bundled extern.RangeHTTPServer (removed in this PR).

Fixes #798

Checklist:

  • pre-commit checks ran successfully
  • tests ran successfully
  • for a public change, a changelog entry was added
  • for a public change, documentation was updated
  • for any new features, unit tests were added

@braingram braingram changed the title Cleanup helpers Cleanup asdf._tests._helpers Dec 13, 2023
@braingram braingram marked this pull request as ready for review December 13, 2023 21:51
@braingram braingram requested a review from a team as a code owner December 13, 2023 21:51
@braingram braingram requested a review from eslavich December 13, 2023 22:02
@braingram braingram force-pushed the cleanup_helpers branch 3 times, most recently from a471c37 to 5984829 Compare February 14, 2024 15:54
@braingram braingram force-pushed the cleanup_helpers branch 3 times, most recently from 50d4b67 to db99b6d Compare February 27, 2024 14:55
Copy link
Member

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

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

this looks good to me and simplifies the test fixtures; can it be rebased or is this branch too far gone to be worth it?

@braingram
Copy link
Contributor Author

Thanks for the review! I will (attempt to) resolve the conflicts 😬

@braingram braingram merged commit 2a07337 into asdf-format:main May 7, 2024
48 checks passed
@braingram braingram deleted the cleanup_helpers branch May 7, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider replacing vendorized RangeHTTPServer
2 participants