Skip to content

Fix structure and execution of type tests#18594

Merged
martint merged 1 commit intotrinodb:masterfrom
martint:type-tests
Aug 10, 2023
Merged

Fix structure and execution of type tests#18594
martint merged 1 commit intotrinodb:masterfrom
martint:type-tests

Conversation

@martint
Copy link
Copy Markdown
Member

@martint martint commented Aug 8, 2023

Do not use inheritance/overriding for code reuse and specialization, as it makes it harder to reason about what's being tested.

Also, annotate the tests directly so that they are picked up by JUnit

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

Do not use inheritance/overriding for code reuse and specialization,
as it makes it harder to reason about what's being tested.

Also, annotate the tests directly so that they are picked up by JUnit
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

how do we plan to deal with evolving tests? e.g. if you add a new test testRange then how do you identify which all test classes it should go into? Similarly how do you ensure there isn't a drift in the test method definition across classes.

@martint
Copy link
Copy Markdown
Member Author

martint commented Aug 9, 2023

Each type needs tests according to its semantics. If you add new behavior to a type that affects how getRange() works, the corresponding tests for that type need to be adjusted. That's no different than how we implement tests for timestamp, timestamp w/ tz, time, time w/ tz, etc. in io.trino.operator.scalar.timestamp.TestTimestamp, io.trino.operator.scalar.timestamp.TestTime, and friends.

@martint martint merged commit a5c50dc into trinodb:master Aug 10, 2023
@martint martint deleted the type-tests branch August 10, 2023 22:29
@martint martint mentioned this pull request Oct 26, 2023
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants