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

GraphExploreResponseTests XContent Tests failing #33686

Closed
hub-cap opened this issue Sep 13, 2018 · 10 comments
Closed

GraphExploreResponseTests XContent Tests failing #33686

hub-cap opened this issue Sep 13, 2018 · 10 comments
Assignees
Labels
>test-failure Triaged test failures from CI v6.5.0 v7.0.0-beta1

Comments

@hub-cap
Copy link
Contributor

hub-cap commented Sep 13, 2018

I have seen a few of these fail on different tests. Repros are below. I did not test on 7.0, but this did happen on 6.x

./gradlew :x-pack:protocol:test \
  -Dtests.seed=725399AD44911E5E \
  -Dtests.class=org.elasticsearch.protocol.xpack.graph.GraphExploreResponseTests \
  -Dtests.security.manager=true

./gradlew :x-pack:protocol:test \
  -Dtests.seed=A9994E345900D947 \
  -Dtests.class=org.elasticsearch.protocol.xpack.graph.GraphExploreResponseTests \
  -Dtests.method="testFromXContent" \
  -Dtests.security.manager=true
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@hub-cap hub-cap added >test-failure Triaged test failures from CI and removed >bug labels Sep 13, 2018
@markharwood markharwood self-assigned this Sep 14, 2018
markharwood added a commit to markharwood/elasticsearch that referenced this issue Sep 14, 2018
markharwood added a commit that referenced this issue Sep 14, 2018
…ap insertion sequence (#33709)

Solved by sorting the vertices arrays before comparison
Closes #33686
markharwood added a commit that referenced this issue Sep 14, 2018
…ap insertion sequence (#33709)

Solved by sorting the vertices arrays before comparison
Closes #33686
@s1monw
Copy link
Contributor

s1monw commented Sep 19, 2018

@markharwood this seems to fail again but on a different (same?) test in a different project / dir

./gradlew :x-pack:protocol:test -Dtests.seed=B915FE5C0EE5813C -Dtests.class=org.elasticsearch.protocol.xpack.graph.GraphExploreResponseTests -Dtests.method="testFromXContentWithFailures" -Dtests.security.manager=true -Dtests.locale=th-TH -Dtests.timezone=Europe/Vienna -Dcompiler.java=10 -Druntime.java=8

can you please have a look? It seems this test is a copy and didn't get you fix from #33709

@s1monw s1monw reopened this Sep 19, 2018
@markharwood
Copy link
Contributor

@hub-cap Can you give any insight on the need to maintain 2 copies of this test?

markharwood added a commit that referenced this issue Sep 19, 2018
Graph connections could appear in different orders based on insertion sequence

Closes #33686
@markharwood
Copy link
Contributor

Added fixes for 6.x and master.
We still have 2 identical classes that are being maintained.
The fixes were:

  • The "xpack/plugin/core" was missing the Vertex sorting logic previously added to "xpack/protocol" version.
  • Both classes were missing Connection sorting logic which was the basis of the latest failure reported above by Simon.

These classes are now in sync on 6.x and master but the question remains do we need them both and for how long?

@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 19, 2018

We should not have 2 copies of the test. Do we have 2 copes of the response?

@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 19, 2018

ok this is still one of the things i need to fix up, this was one of the request/responses copied into protocol, and copied back when we severed the tie between protocol and client. There is still a cleanup task for me to go back and remove protocol, but im working on splitting the client itself up still. Everything in protocol will go away, and im going to revert back to what the actions looked like in their original locations in the core plugin, so if something is messing up in protocol, nuke it. itll be cleaned up when i nuke it all :)

@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 19, 2018

I somewhat misspoke. The previous sentence assumes you have some request/response in the HLRC, instead. These classes contain much less than their server counterparts, so they can be simpler. I vote that you move those into HLRC, and revert any changes to the original ones in x-pack (or i will end up doing this eventually hehe)

@andrershov
Copy link
Contributor

@markharwood testFromXContent still fails on 6.x and master (because of different vertices order), can you please take a look?
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-unix-compatibility/os=centos/52/console
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-unix-compatibility/os=centos/52/console

@andrershov andrershov reopened this Oct 15, 2018
@andrershov
Copy link
Contributor

andrershov commented Oct 15, 2018

On both 6.x and master

REPRODUCE WITH: ./gradlew :x-pack:plugin:core:test \
  -Dtests.seed=C428B114B7E093FA \
  -Dtests.class=org.elasticsearch.protocol.xpack.graph.GraphExploreResponseTests \
  -Dtests.method="testFromXContent" \
  -Dtests.security.manager=true \
  -Dtests.locale=sr-Latn \
  -Dtests.timezone=Brazil/Acre \
  -Dcompiler.java=11 \
  -Druntime.java=8

REPRODUCE WITH: ./gradlew :x-pack:plugin:core:test \
  -Dtests.seed=B70EEBBDA5FE2884 \
  -Dtests.class=org.elasticsearch.protocol.xpack.graph.GraphExploreResponseTests \
  -Dtests.method="testFromXContent" \
  -Dtests.security.manager=true \
  -Dtests.locale=id \
  -Dtests.timezone=America/Miquelon \
  -Dcompiler.java=11 \
  -Druntime.java=8

markharwood added a commit to markharwood/elasticsearch that referenced this issue Oct 15, 2018
…rtions but the parsed objects’ .equals() methods have the sort logic required to prove connections and vertices are correct. Disabled the xContent equivalence checks.

Closes elastic#33686
markharwood added a commit that referenced this issue Oct 16, 2018
…34473)

xContent ordering is unreliable when derived from map insertions but the parsed objects’ .equals() methods have the sort logic required to prove connections and vertices are correct. Disabled the xContent equivalence checks. 

Closes #33686
markharwood added a commit that referenced this issue Oct 16, 2018
…34473)

xContent ordering is unreliable when derived from map insertions but the parsed objects’ .equals() methods have the sort logic required to prove connections and vertices are correct. Disabled the xContent equivalence checks.

Closes #33686
@markharwood
Copy link
Contributor

Addressed on master 75c973f
and 6.x 9d4c438

markharwood added a commit to markharwood/elasticsearch that referenced this issue Oct 26, 2018
API re-uses the GetRequest object (following the precedent set by the plain “exists” api).

Relates to elastic#33686
kcm pushed a commit that referenced this issue Oct 30, 2018
…34473)

xContent ordering is unreliable when derived from map insertions but the parsed objects’ .equals() methods have the sort logic required to prove connections and vertices are correct. Disabled the xContent equivalence checks. 

Closes #33686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>test-failure Triaged test failures from CI v6.5.0 v7.0.0-beta1
Projects
None yet
Development

No branches or pull requests

6 participants