Conversation
These are covered in engine tests.
martint
force-pushed
the
product-tests
branch
from
89c66d1 to
f169b0f
Compare
findepi
left a comment
findepi
left a comment
There was a problem hiding this comment.
"Move Teradata function tests to unit tests"
| @Test(groups = FUNCTIONS) | ||
| public void testIndex() | ||
| { | ||
| assertThat(onTrino().executeQuery("SELECT index('high', 'ig')")).contains(row(2)); |
There was a problem hiding this comment.
The advantage of this over a unit test was
- ensuring the td functions are part of the production bundle.
- for plugins, we manage to violate this many times over, so it's a lesson learned
- ensuring the td functions work without
trino-mainon the classpath- for functions, we managed to make them work in unit tests only. I remember a case where one of the maintainers fixed plugin-provided aggregate functions 3 times, because we lacked product test coverage
I think we should keep at least one product tests' test case
There was a problem hiding this comment.
What setups do these tests run with? Are they executed with all the combinations of connectors and environments?
There was a problem hiding this comment.
i hope not! Once should be enough (and i guess this was the case)
findepi
left a comment
findepi
left a comment
There was a problem hiding this comment.
"Remove unnecessary product tests" LGTM
| @Test(groups = {COMPARISON, QUERY_ENGINE}, dataProvider = "operands") | ||
| public void testLessThanOrEqualOperatorExists(String leftOperand, String rightOperand, String typeName) | ||
| { | ||
| assertThat(onTrino().executeQuery(format("select cast(%s as %s) <= cast(%s as %s)", leftOperand, typeName, rightOperand, typeName))) |
There was a problem hiding this comment.
We definitely should retain some smoke test coverage of the engine as part of the product tests, to ensure Trino works in its final classpath/classloaders composition.
I don't believe such a detailed test like this was really meant for that, so I am fine with the removal.
It would be good to known what are the tests we want to retain. Maybe we don't have them yet?
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Consider this a reminder @martint ... I assume you will merge this after the 436 release. |
| @Test(groups = FUNCTIONS) | ||
| public void testIndex() | ||
| { | ||
| assertThat(onTrino().executeQuery("SELECT index('high', 'ig')")).contains(row(2)); |
findepi
dismissed
their stale review
(in case i don't have time to re-review)
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
I assume this is still valid and you will proceed with @martint |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
4 participants
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.