Skip to content

SQL: [Tests] Mute testReplaceChildren for Pivot #48997

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

Closed
wants to merge 1 commit into from

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Nov 12, 2019

Temporarily "mute" the testReplaceChildren for Pivot since it leads to
failing tests for some seeds, since the new child doesn't respond to a
valid data type.

Relates to #48900

Temporarily "mute" the testReplaceChildren for Pivot since it leads to
failing tests for some seeds, since the new child doesn't respond to a
valid data type.

Relates to elastic#48900
@matriv matriv added >test-failure Triaged test failures from CI :Analytics/SQL SQL querying v8.0.0 v7.6.0 labels Nov 12, 2019
@matriv matriv requested review from costin and astefan November 12, 2019 16:38
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

@matriv
Copy link
Contributor Author

matriv commented Nov 12, 2019

I believe a cleaner approach would be the use of JUnit's Assume class. One example is https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/SqlSpecTestCase.java#L78-L84.

I'm sorry I don't get how it can improve it, this method is run with every class extending Node as a subclass.

@matriv
Copy link
Contributor Author

matriv commented Nov 12, 2019

Closing in favour of a proper fix: #49004

@matriv matriv closed this Nov 12, 2019
@astefan
Copy link
Contributor

astefan commented Nov 12, 2019

Instead of using the return when that condition was met, an Assume.assumeFalse(subclass.equals(Pivot.class)) would have been enough. It wasn't about the class checking itself, but about the test's assumption that a certain condition should be met through the use of Assume.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying >test-failure Triaged test failures from CI v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants