Reenable PingAndInfoIT.testXPackInfo() on 6.x #38168
Conversation
The version exposed in ML feature set are extracted from the ML engine. The format of the version seems to have changed and it different for dev vs tagged version of the engine. This PR unmutes the test and makes it a bit more permissive about the format of the exposed version. Closes elastic#34386
tlrx
added
>test
|
Pinging @elastic/es-core-features |
tlrx
changed the title
| assertEquals(mainResponse.getVersion().toString(), | ||
| ml.nativeCodeInfo().get("version").toString().replace("-SNAPSHOT", "")); | ||
| assertThat(ml.nativeCodeInfo().get("version").toString().replace("-SNAPSHOT", ""), | ||
| anyOf(equalTo(mainResponse.getVersion().toString()), startsWith("based on " + mainResponse.getVersion().toString()))); |
There was a problem hiding this comment.
I don't think this test should tolerate "based on". The ml-cpp build system adds "based on" to the version when it builds a repo with uncommitted changes. CI should never be testing ml-cpp builds with uncommitted changes, and we should never have uploaded such a build to the official download location on S3.
Please can you try changing this PR to just unmute the test, and leave the assertion as it was before. If this test then fails in the PR build then I'll take over and find out what is wrong now.
This test was muted around 10th October 2018, when we switched over from building ml-cpp snapshots on the old Prelert Jenkins to building on infra's Jenkins at elasticsearch-ci.elastic.co. For a few days there was a problem where ml-cpp built within a Docker container on infra's setup had a crazy idea of the state of the git repo within the Docker container because it was relying on symlinks to directories outside of the Docker container to optimize the cloning process (inherited from the elasticsearch repo's worker setup). We fixed this in elastic/ml-cpp#270 after debugging using elastic/ml-cpp#269. The full story is in elastic/infra#7464.
It's unfortunate that nobody in the ML team was pinged when this test was muted, as we could have dealt with unmuting at the same time as we fixed the underlying problem. (In fact, you can see from the dates of those other issues and PRs that I linked that we didn't notice there was a problem for a couple of days after the ES test failure revealed it.)
There was a problem hiding this comment.
Thanks David for explaining the whole story around this. I wasn't sure about the tagged/dev differences and that's why I pinged you directly.
Please can you try changing this PR to just unmute the test, and leave the assertion as it was before. If this test then fails in the PR build then I'll take over and find out what is wrong now.
Done. It doesn't fail on master so I expect it to be fine in 6.x too.
It's unfortunate that nobody in the ML team was pinged when this test was muted, as we could have dealt with unmuting at the same time as we fixed the underlying problem.
I agree. But this kind of things happen when we have to deal with a large number of failures :(
|
Pinging @elastic/ml-core |
|
@droberts195 I updated the test according to your comment. Can you have another look please? |
droberts195
left a comment
droberts195
left a comment
There was a problem hiding this comment.
LGTM assuming that test passes now
|
Thanks for working on this @tlrx! |
|
@elasticmachine run elasticsearch-ci/packaging-sample |
3 participants
The version exposed in ML feature set are extracted from the ML engine.
The format of the version have changed (see #38168 (review)) .
This PR unmutes the test.
Closes #34386