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

deprecate asdf.tests.helpers #1440

Merged
merged 8 commits into from
Feb 23, 2023

Conversation

braingram
Copy link
Contributor

The current users of asdf.tests.helpers (outside this repo) are (with 1 example from each):

The uses in the deprecated astropy.io.misc.asdf are ignored.

specutils uses assert_roundtrip_tree to test that object serialization then deserialization results in an equivalent object. asdf.testing.roundtrip_object can be used but requires either using the assert_equal class method defined on the spectra, implementation of an equality operator or a similar test to verify that the deserialized object is 'equal' (it currently relies on a feature of assert_roundtrip_tree which checks for the assert_equal class method of the custom type).

asdf-astropy uses yaml_to_asdf. An alternative, drop in replacement exists in asdf.testing.helpers.

@braingram braingram marked this pull request as ready for review February 23, 2023 17:06
@braingram braingram requested a review from a team as a code owner February 23, 2023 17:06
tox.ini Outdated Show resolved Hide resolved
asdf/tests/_helpers.py Show resolved Hide resolved
@WilliamJamieson
Copy link
Contributor

specutils uses assert_roundtrip_tree to test that object serialization then deserialization results in an equivalent object. asdf.testing.roundtrip_object can be used but requires either using the assert_equal class method defined on the spectra, implementation of an equality operator or a similar test to verify that the deserialized object is 'equal' (it currently relies on a feature of assert_roundtrip_tree which checks for the assert_equal class method of the custom type).

asdf-astropy uses yaml_to_asdf. An alternative, drop in replacement exists in asdf.testing.helpers.

If this is the case, please open PRs on specutils and asdf-astropy implementing these fixes. I would like to review those changes first before I consider the changes in this PR.

Also, have you checked the if the new warnings are raised in the rest of the downstream tests? The three packages you list here are simply the ones which turn warnings into errors. I would like to cover off these changes in the rest of the downstream.

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

After an offline conversation with @braingram, it was explained that his analysis indicates that they only downstream uses of asdf.tests.helpers were in astropy, specutils, and asdf-astropy which already have solution paths planned.

Given this analysis, I retract my previous comments (aside from removing the single suppression from the downstream tox).

These changes look good to me otherwise.

braingram added a commit to braingram/asdf-astropy that referenced this pull request Feb 23, 2023
braingram added a commit to braingram/specutils that referenced this pull request Feb 23, 2023
asdf.tests.helpers will be deprecated:
asdf-format/asdf#1440

The uses in specutils are limited to assert_roundtrip_tree

A similar roundtrip_object exists in asdf.testing.helpers
which serializes then deserializes an object (without asserting
equality). This commit replaces the uses of assert_roundtrip_tree
with roundtrip_object and uses the equality comparison built into
the custom asdf types implemented in this module to check for
equality. These equality comparisons are the same checks that
were used internally by assert_roundtrip_tree.
@braingram
Copy link
Contributor Author

Fixes #1363

@braingram
Copy link
Contributor Author

If this is the case, please open PRs on specutils and asdf-astropy implementing these fixes. I would like to review those changes first before I consider the changes in this PR.

@WilliamJamieson I opened PRs:
astropy/asdf-astropy#168
astropy/specutils#1025

I converted them both to draft in case we'd like to rename testing.helpers. I'm inclined to leave it as is but don't feel strongly. If you'd like to change the name I don't have any objection.

@braingram braingram merged commit 81ab02d into asdf-format:master Feb 23, 2023
meeseeksmachine pushed a commit to meeseeksmachine/asdf that referenced this pull request Feb 23, 2023
github-actions bot pushed a commit that referenced this pull request Feb 23, 2023
@braingram braingram deleted the deprecate/tests_helpers branch February 24, 2023 19:23
pllim pushed a commit to astropy/specutils that referenced this pull request Feb 24, 2023
* change asdf.tests.helpers to asdf.testing.helpers

asdf.tests.helpers will be deprecated:
asdf-format/asdf#1440

The uses in specutils are limited to assert_roundtrip_tree

A similar roundtrip_object exists in asdf.testing.helpers
which serializes then deserializes an object (without asserting
equality). This commit replaces the uses of assert_roundtrip_tree
with roundtrip_object and uses the equality comparison built into
the custom asdf types implemented in this module to check for
equality. These equality comparisons are the same checks that
were used internally by assert_roundtrip_tree.

* fix codestyle and typo in xfailed test

* add comment with github link to xfailed SpectralAxis test
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.

2 participants