Skip to content

SQL: [Tests] Mute testReplaceChildren for Pivot#48997

Closed
matriv wants to merge 1 commit intoelastic:masterfrom
matriv:temp-fix-48900
Closed

SQL: [Tests] Mute testReplaceChildren for Pivot#48997
matriv wants to merge 1 commit intoelastic:masterfrom
matriv:temp-fix-48900

Conversation

@matriv
Copy link
Copy Markdown
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

@matriv
SQL: [Tests] Mute testReplaceChildren for Pivot
309e00f 
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 astefan and costin November 12, 2019 16:38
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Nov 12, 2019

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

astefan
astefan reviewed Nov 12, 2019
View reviewed changes
Copy link
Copy Markdown
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.

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.

@matriv
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@matriv matriv mentioned this pull request Nov 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astefan astefan astefan left review comments

@costin costin Awaiting requested review from costin

Assignees

No one assigned

Labels

:Analytics/SQL SQL querying >test-failure Triaged test failures from CI v7.6.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@matriv @elasticmachine @astefan @jakelandis