refactor `test_connect` for new test suite #12191

nstarman · 2021-09-23T21:11:22Z

Provides mixins for TestCosmology to test the descriptors.
Now every cosmology will test that it can I/O.
Also the classes in connect.py have some extra tests built upon the more general tests that each cosmology runs.

Signed-off-by: Nathaniel Starkman (@nstarman) nstarkman@protonmail.com

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

pep8speaks · 2021-09-23T21:11:26Z

Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-09-24 21:06:23 UTC

mhvk

This looks good. The one thing I feel is missing is good docstrings for the Mixin test classes, on what subclasses are supposed to define (the cosmo fixture?).

One thing I wondered was whether you are still sure no test is run twice - a disadvantage of this fairly complicated structure is that this is no longer obvious...

Anyway, I think the goal here is good and it looks all in order, so approving.

mhvk · 2021-09-24T10:15:04Z

astropy/cosmology/core.py

@@ -264,7 +264,7 @@ def __new__(cls, *args, **kwargs):

        return self

-    def __init__(self, *args, name=None, meta=None, **kwargs):
+    def __init__(self, name=None, meta=None):


Yes, good not to eat arguments without warning or error!

mhvk · 2021-09-24T10:17:12Z

astropy/cosmology/tests/test_connect.py


-class TestReadWriteCosmology:
+    @pytest.fixture(scope="class", autouse=True)


Didn't know about this one. Out of curiosity, any reason not to use setup_class and teardown_class? Are you avoiding those to avoid forcing super() calls when actually used as a mixin?

Yeah, it's mostly that.
And since these are "mixin" it helps keep the class orthogonal.

mhvk · 2021-09-24T10:19:25Z

astropy/cosmology/tests/test_connect.py

        """
        Test writing from an instance and reading from that class.
        This requires partial information.

        .. todo::

-            generalize over all save formats for this test.
+            remove when fix method in super


The comment is addressed to yourself, so perhaps not relevant, but I would have no idea what it meant!

Ah yeah. I haven't figured out how to do this in general. Modifying a dict is different than a table is different than a Model. I want to test reading from a class and having that class fill in missing information, but I need to figure out how to delete that information. So currently this test only runs on dicts.

It's not dire, which is why I'm leaving in a TODO, but it would be best to show the information inference works irrespective of the format.

I think I have a solution... you'll see it soonish 😆

nstarman · 2021-09-24T15:49:12Z

This looks good. The one thing I feel is missing is good docstrings for the Mixin test classes, on what subclasses are supposed to define (the cosmo fixture?).

A good point! I'll add some documentation on that.
Yes, subclasses just need to define a cosmo fixture. Thankfully pytest remembers all the other fixtures in inherited methods.

One thing I wondered was whether you are still sure no test is run twice - a disadvantage of this fairly complicated structure is that this is no longer obvious...

I've tested this pretty thoroughly in other code. Long story short: pytest only runs code starting with test (case insensitive), but it will run a test suite twice if that code is imported into a different module. So a test test_core.TestThis imported into test_flrw.py will be run a second time. There is an easy way around this: rename the imported object, e.g. from .test_core import TestCosmology as CosmologyTest. CosmologyTest is not run again.
It's contents will be run when stuck into a subclass, but that's on purpose since the subclass provides a new cosmo fixture.

@nstarman

Provides mixins for TestCosmology to test the descriptors Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman added this to the v5.0 milestone Sep 23, 2021

nstarman added this to In progress in Cosmology, the Expansion via automation Sep 23, 2021

nstarman force-pushed the cosmo_improve_connect_tests branch from ba21596 to 5f1a7c6 Compare September 23, 2021 21:11

github-actions bot added cosmology testing labels Sep 23, 2021

nstarman force-pushed the cosmo_improve_connect_tests branch from 5f1a7c6 to 312a7f7 Compare September 23, 2021 21:12

nstarman added the Refactoring label Sep 23, 2021

nstarman requested a review from mhvk September 23, 2021 21:55

nstarman force-pushed the cosmo_improve_connect_tests branch from 312a7f7 to 2b4ffb5 Compare September 24, 2021 01:36

mhvk approved these changes Sep 24, 2021

View reviewed changes

Cosmology, the Expansion automation moved this from In progress to Reviewer approved Sep 24, 2021

nstarman force-pushed the cosmo_improve_connect_tests branch from 2b4ffb5 to e1c445d Compare September 24, 2021 15:51

nstarman added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Sep 24, 2021

refactor test_connect for new test suite

2160c2b

Provides mixins for TestCosmology to test the descriptors Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman force-pushed the cosmo_improve_connect_tests branch from e1c445d to 2160c2b Compare September 24, 2021 21:06

nstarman merged commit e078885 into astropy:main Sep 24, 2021

Cosmology, the Expansion automation moved this from Reviewer approved to Done Sep 24, 2021

nstarman deleted the cosmo_improve_connect_tests branch September 24, 2021 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor `test_connect` for new test suite #12191

refactor `test_connect` for new test suite #12191

nstarman commented Sep 23, 2021 •

edited

pep8speaks commented Sep 23, 2021 •

edited

mhvk left a comment

mhvk Sep 24, 2021

mhvk Sep 24, 2021

nstarman Sep 24, 2021

mhvk Sep 24, 2021

nstarman Sep 24, 2021

nstarman Sep 24, 2021

nstarman Sep 24, 2021

nstarman commented Sep 24, 2021 •

edited


		class TestReadWriteCosmology:
		@pytest.fixture(scope="class", autouse=True)

refactor test_connect for new test suite #12191

refactor test_connect for new test suite #12191

Conversation

nstarman commented Sep 23, 2021 • edited

Checklist for package maintainer(s)

pep8speaks commented Sep 23, 2021 • edited

Comment last updated at 2021-09-24 21:06:23 UTC

mhvk left a comment

Choose a reason for hiding this comment

mhvk Sep 24, 2021

Choose a reason for hiding this comment

mhvk Sep 24, 2021

Choose a reason for hiding this comment

nstarman Sep 24, 2021

Choose a reason for hiding this comment

mhvk Sep 24, 2021

Choose a reason for hiding this comment

nstarman Sep 24, 2021

Choose a reason for hiding this comment

nstarman Sep 24, 2021

Choose a reason for hiding this comment

nstarman Sep 24, 2021

Choose a reason for hiding this comment

nstarman commented Sep 24, 2021 • edited

refactor `test_connect` for new test suite #12191

refactor `test_connect` for new test suite #12191

nstarman commented Sep 23, 2021 •

edited

pep8speaks commented Sep 23, 2021 •

edited

nstarman commented Sep 24, 2021 •

edited