Skip to content

Reenable PingAndInfoIT.testXPackInfo() on 6.x #38168

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.

Sign up for GitHub

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

Merged
tlrx merged 2 commits into from
Feb 1, 2019
Merged

Reenable PingAndInfoIT.testXPackInfo() on 6.x #38168

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ccb6a7
Reenable PingAndInfoIT on 6.x
tlrx Feb 1, 2019
db5efa2
Apply feedback
tlrx Feb 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/PingAndInfoIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
import java.util.EnumSet;
import java.util.Map;

import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.startsWith;

public class PingAndInfoIT extends ESRestHighLevelClientTestCase {

public void testPing() throws IOException {
Expand All @@ -56,7 +60,6 @@ public void testInfo() throws IOException {
assertEquals(versionMap.get("lucene_version"), info.getVersion().luceneVersion.toString());
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/34386")
public void testXPackInfo() throws IOException {
XPackInfoRequest request = new XPackInfoRequest();
request.setCategories(EnumSet.allOf(XPackInfoRequest.Category.class));
Expand Down Expand Up @@ -85,8 +88,8 @@ public void testXPackInfo() throws IOException {
assertNotNull(ml.description());
assertTrue(ml.available());
assertTrue(ml.enabled());
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())));
Copy link

@droberts195 droberts195 Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link
Member Author

@tlrx tlrx Feb 1, 2019

Choose a reason for hiding this comment

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

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 :(

}

public void testXPackInfoEmptyRequest() throws IOException {
Expand Down