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

Tests restructure proposal #268

Merged
merged 10 commits into from Oct 4, 2023
Merged

Tests restructure proposal #268

merged 10 commits into from Oct 4, 2023

Conversation

ianspektor
Copy link
Collaborator

@ianspektor ianspektor commented Sep 25, 2023

Proposal for restructuring tests with a single op (as discussed last meeting):

  • all tests moved to core and centralized in a single file
  • all tests call the API method
  • tests no longer use the pandas constructor, nor need to assemble numpy/core classes with nodes and such
  • tests for base class (e.g. base window op logic in this case) in separate file, since its shared for all its children and we don't want to test it in each
  • used @parametrize which decreases amount of work and code by a lot, but makes the actual code in the test harder to understand (this case is especially hard though because of needing to use different samplings and window lengths and such depending on what was received as params). what do you think?

migrating this for a single op was about 2h, but its the first one (so lots of time invested in deciding what to do) and it was a well-tested one. I'd expect the mean time to migrate an arbitrary op to be around 30m worst case, which sums up to about 1 week of full time work for our 75 ops :)

@ianspektor
Copy link
Collaborator Author

@achoum @DonBraulio based on last call:

  • added moving_sum and cumsum tests to check if refactoring/abstracting code made sense.
  • created temporian.test.utils.assertOperatorResult that checks an op's output (and checks (de)serialization of the graph, this is the generic method to be extended and use in all ops)
  • decided not to document the @parameters params, instead removed all logic from test in the simple moving average's by splitting the big method into several smaller ones. let me know if this works or you'd still rather have them documented, I feel its something we won't want to maintain/enforce

@ianspektor
Copy link
Collaborator Author

ianspektor commented Oct 2, 2023

had to rename the test_util.py test since it was being ran as a test which made the diff very polluted. the test files that should be reviewed are just the new ones under temporian/core/operators/window/test

@ianspektor ianspektor merged commit 290a4ea into main Oct 4, 2023
17 checks passed
@ianspektor ianspektor deleted the restructure-tests branch October 4, 2023 12:30
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.

None yet

2 participants