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

PLAT-163: Migrate test_relational_operand #1135

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

ethho
Copy link
Contributor

@ethho ethho commented Dec 14, 2023

  • cp to tests
  • nose2pytest test_relational_operand
  • First pass at migrating test_relational_operand
  • Format with black

@@ -147,7 +147,7 @@ def make(self, key):
from datetime import date, timedelta

users = [None, None] + list(User().fetch()["username"])
random.seed("Amazing Seed")
random.seed("Amazing Seed4")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For test_restriction_by_null, we needed some users in this table with name None. I changed the seed until the table contents fulfilled this restriction, and test_restriction_by_null passed.



def test_populate(schema_simp_pop):
assert not B().progress(display=False)[0], "B incompletely populated"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not separate B() into a separate fixture in this module. In my opinion, the best practice that balances consistency and readability is:

  • If the table instance was being instantiated in a setup_class in the old tests (like cls.subject = Subject()), then create and use a subject fixture instead. In these cases, the original test authors used self.subject instead of Subject() in the tests.
  • If the table instance is being constructed inline, as is the case for B() in this module, then the original test authors weren't worried about state that carried over between tests, and we can leave the inline construction.

Will gladly take feedback on this convention (which I'm aware is changing as we go through). I could replace every B() with a fixture b, but I think this would make the module much less readable, even if it's more consistent with other modules that need fixtures for this.

@ethho ethho requested a review from A-Baji December 14, 2023 17:14
@ethho ethho marked this pull request as ready for review December 14, 2023 17:14
@A-Baji A-Baji merged commit 20c5039 into datajoint:dev-tests Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants