[test] port archive distribution packaging tests #31314

andyb-elastic · 2018-06-14T02:32:58Z

Recreates the rest of the bats packaging tests for the tar distribution
in the java packaging test project, with support for both tar and zip
packaging, both oss and default flavors, and on Linux and Windows. Most
tests are followed fairly closely, some have either been dropped if
unnecessary or folded into others if convenient.

For #26741

elasticmachine · 2018-06-14T02:33:00Z

Pinging @elastic/es-core-infra

Recreates the rest of the bats packaging tests for the tar distribution in the java packaging test project, with support for both tar and zip packaging, both oss and default flavors, and on Linux and Windows. Most tests are followed fairly closely, some have either been dropped if unnecessary or folded into others if convenient.

nik9000 · 2018-06-18T18:29:50Z

qa/vagrant/build.gradle

@@ -31,6 +31,12 @@ dependencies {
  compile "org.hamcrest:hamcrest-core:${versions.hamcrest}"
  compile "org.hamcrest:hamcrest-library:${versions.hamcrest}"

+  compile "org.apache.httpcomponents:httpcore:${versions.httpcore}"
+  compile "org.apache.httpcomponents:httpclient:${versions.httpclient}"
+  compile "org.apache.httpcomponents:fluent-hc:${versions.httpclient}"


Note to self to check on this.

Usage of httpcomponents' client in general, or the fluent api? I just added the latter to reduce the verbosity a little but don't mind removing it

The fluent API.

nik9000 · 2018-06-18T20:54:42Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+
+            final Shell sh = new Shell();
+            if (Platforms.WINDOWS) {
+                sh.powershell(


I think it'd be safe to use Java code for this regardless of OS. For some of these things it is great that we're shelling out because we're emulating a real person, but not I don't think a person is going to type this into powershell.

I'm not sure we can change these permissions on windows in java because of how it exposes filesystem attributes for windows' fs. Or at least that was what I originally intended to do but couldn't get it working. Is there something you could point me towards?

But yeah definitely right about the user not doing it this way, they'll use the gui

nik9000 · 2018-06-18T20:56:54Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Archives.java

+            // these tests run as Administrator. we don't want to run the server as Administrator, so we provide the current user's
+            // username and password to the process which has the effect of starting it not as Administrator.
+            sh.powershell(
+                "$password = ConvertTo-SecureString 'vagrant' -AsPlainText -Force; " +


My experience has been that one line of bash converts roughly to ten lines of powershell

nik9000 · 2018-06-18T20:58:39Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+                break;
+
+            } catch (HttpHostConnectException e) {
+                // we want to retry if the connection is refused, so do nothing here


nik9000 · 2018-06-18T20:59:01Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+            }
+
+            try {
+                Thread.sleep(1000);


A second feels like a fairly long time to wait.

I'm no expert, but I buy the argument here that you should probably re-interrupt the thread just in case. The runtime exception is fine though because you want the test to fail.

Honestly why not let the InterruptedException bubble out and let the test framework catch it and do whatever it thinks is right with it?

I would argue against no sleep at all. If we can make the tests that much faster burning the CPU is worth it. Hammering the port during startup might even uncover something interesting. Would probably need to switch from to a time based loop instead of a number of retries based one.

Yeah where I didn't have strong opinions I just followed what the bats tests did. 1 second intervals with 120 retries seems a little excessive

elasticsearch/qa/vagrant/src/test/resources/packaging/utils/utils.bash

Line 482 in 631a53a

wget -O - --retry-connrefused --waitretry=1 --timeout=120 --tries=120 http://localhost:9200/_cluster/health || {

I would argue that we do want to set some lower time bound on how long we wait for the server to come up, because that's always going to take significantly longer than what we're doing here whether we retry in a loop or in httpclient. That is, while the server startup time varies, it's never going to occasionally be only a millisecond. Does that make sense or am I misunderstanding?

My thinking on InterruptedException was just that we should fail fast, and that I didn't want to add it to the signature of every method in the call stack. That's a good point about letting junit handle it - I took a (not very deep) look and it doesn't seem like junit 4 does anything interesting with it. I'll think a little more about what to do here but I'll at least reset the interrupt on the thread

The server will take some time to come up for sure, but why try to guess how much that is, when the cost of not doing so is not significant. If this were a communication in production, then we would have to do all these things to be nice to the server, but since this is a test, we don't need to be nice. All we need is to measure how long we have been trying to connect for and time out if it's too long, but we don't need necessarily need to add delays in between.

I don't think not dealing with the interrupt is is not likely to interfere with junit, I never saw it do, but it's a bad practice in general, as the interrupt is never reset, it gets lost and the thread that was interrupted will keep on running. I found this to be a big deal when it's in some utility or library that the thread calls, and then one is left to wonder why graceful termination is not possible elsewhere

Gotcha, I'll remove the sleep and terminate the waiting based on directly measuring the time spent

nik9000 · 2018-06-18T21:05:12Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+            }
+
+            return body;
+        } catch (IOException e) {


I tend to be fine declaring that my tests throw IOException or even throw Exception. So I would just let these bubble out.

alpar-t · 2018-06-25T05:31:26Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+        final Shell sh = new Shell();
+        final Result result;
+        if (Platforms.WINDOWS) {
+            result = sh.powershell(installation.bin("elasticsearch-keystore.bat") + " list");


I'm wondering if it would make sense to have an abstraction on top of Shell to call bin scripts in a platform independent way. I think it would make tests more readable e.x. having something like ESShell().kestore().list() to achieve this. Or maybe EsShell().escalatePrivileges().keystore().list() to make it run with administrator/root

Agree, though I'd lean towards just doing it for the executable paths

alpar-t · 2018-06-25T05:41:05Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+            append(tempConf.resolve("elasticsearch.yml"), "node.name: relative");
+
+            final Shell sh = new Shell();
+            if (Platforms.WINDOWS) {


This conditional is repeated a few times and not very readable, I might consider extracting it to a utility method.
Would also consider adding Platform.onWindowsOnly(PlatformSpecificAction do, String description) where PlatformSpecificAction is just a functional interface, I would find that more readable, and besides the else condition is not technically correct java as not windows doesn't imply posix ( in theory at least ). It would also give us the opportunity to log that desciption was carried out, or it wasn't on thi platform.

I've mostly tried to keep the control flow as simple as possible in cases like this where we're very unlikely to need to extend it any further - that is, even though the if (windows) {} else {} is gross, the effect is really clear and it's unlikely to ever need a third code path.

I like what you've suggested here though and I'll try it out.

the else condition is not technically correct java as not windows doesn't imply posix ( in theory at least )

Agree, this has consistently bothered me. Similarly though, making more of a distinction than that has seemed like overkill

Yes, it would have been an overkill and useless distraction in a conditional, that's also why I suggested abstracting the platform specifics a bit more. Assuming that the platform abstractions are correct, most of the test code doesn't care about the platform, having these conditionals only when there is an actual difference in behavior between these platforms that we need to test for would make it a much easier read and imho help allot in maintaining these tests.

alpar-t · 2018-06-25T05:46:00Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+                break;
+
+            } catch (HttpHostConnectException e) {
+                // we want to retry if the connection is refused, so do nothing here


alpar-t · 2018-06-25T05:53:20Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+            }
+
+            try {
+                Thread.sleep(1000);


I would argue against no sleep at all. If we can make the tests that much faster burning the CPU is worth it. Hammering the port during startup might even uncover something interesting. Would probably need to switch from to a time based loop instead of a number of retries based one.

alpar-t · 2018-06-25T05:55:10Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+            }
+
+            return body;
+        } catch (IOException e) {


andyb-elastic · 2018-07-04T00:35:36Z

@nik9000 @atorok I think I addressed everything mentioned here

One downside of the way this sidesteps all the if (Platforms.WINDOWS) conditionals is that working inside lambdas restricts its ability to do things with the enclosing scope, but there were only a few places where this mattered

alpar-t

LGTM overall, left a few minor comments

alpar-t · 2018-07-04T05:31:14Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

-        final Result r = Platforms.WINDOWS
-            ? sh.powershell(installation.bin("elasticsearch-plugin.bat") + " list")
-            : sh.bash(installation.bin("elasticsearch-plugin") + " list");
+        final Result r = sh.run(bin.elasticsearchPlugin + " list");


++ I think this reads much better.
This is probably a matter of preference, but I think making sh a field and just having something like this would read even better:

assertThat( sh.run( installation.executables().elasticsearchPlugin, "list" ).stdout, isEmptyString() )

Admittedly I personally hate variables used only once.
This will also need a sh.run(String executable, String... args) that joins args with a space.

This will also need a sh.run(String executable, String... args) that joins args with a space

I considered doing this and decided against it because all the commands are run using "bash", "-c", "[everything passed to Shell.run]", and making it look like it handles arguments separately would be trappy (i.e. this makes it clear that Shell does no quoting/escaping for you)

alpar-t · 2018-07-04T05:40:02Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+            final String javaPath = sh.run("which java").stdout.trim();
+            sh.run("chmod -x '" + javaPath + "'");
+            final Result runResult = sh.runIgnoreExitCode(bin.elasticsearch.toString());
+            sh.run("chmod +x '" + javaPath + "'");


maybe have this in a finally

alpar-t · 2018-07-04T05:43:13Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+        });
+
+        // cleanup for next test
+        rm(installation.config("elasticsearch.keystore"));


May be better to do it in @After or move the cleanup to be the first thing, assuming that a previous test somehow left it there it would still work.

I'll move it to the next test, that reflects the way we do cleanup in general (at the beginning rather than at the end).

alpar-t · 2018-07-04T05:58:48Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java

+            assertThat(result.stdout, containsString("keystore.seed"));
+        });
+
+        Platforms.onWindows(() -> {


We could have methods such as onWindows return a platform instance that traces if at least one such method executed, and then have variants that accept a Producer so you would be able to write

String animal = Platform .onWindows(() -> "dinosaur") .onLinux(() -> "penguin")

But we really don't need to go that far. This is fine as it is for now.

Yeah I like the way that reads and did that before, but took it out because it didn't seem necessary

alpar-t · 2018-07-04T06:04:43Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Platforms.java

+        }
+    }
+
+    public static void onLinux(Runnable action) {


I topically declare a @FunctionalInterface for this just to avoid confusion.
Just by looking at this one could assume that the action is carried out in a new thread.

one could assume that the action is carried out in a new thread

That's a good point, I think that's worth adding another type for

alpar-t · 2018-07-04T06:07:56Z

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/ServerUtils.java

+            try {
+
+                final HttpResponse response = Request.Get("http://localhost:9200/_cluster/health")
+                    .connectTimeout((int) waitTime)


I would pass in a fraction of waitTime here or some smaller fixed value.
The initial connect could hang in that timeout and there will be no retries because the outer loop timed out as well.

andyb-elastic · 2018-07-05T18:07:47Z

Thanks for the review @atorok I addressed your comments

alpar-t · 2018-07-06T05:46:08Z

@elasticmachine test this pelase.

alpar-t · 2018-07-06T05:49:20Z

@elasticmachine run all packaging tests

rjernst

LGTM. More bats tests removed!

andyb-elastic · 2018-07-09T20:20:21Z

CI failure looks like opensuse doing its thing. Jenkins run all packaging tests please

Recreates the rest of the bats packaging tests for the tar distribution in the java packaging test project, with support for both tar and zip packaging, both oss and default flavors, and on Linux and Windows. Most tests are followed fairly closely, some have either been dropped if unnecessary or folded into others if convenient.

* master: [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [ML] Re-enable memory limit integration tests (#31328) [test] disable packaging tests for suse boxes Add nio transport to security plugin (#31942) XContentTests : Insert random fields at random positions (#30867) Force execution of fetch tasks (#31974) Fix unreachable error condition in AmazonS3Fixture (#32005) Tests: Fix SearchFieldsIT.testDocValueFields (#31995) Add Expected Reciprocal Rank metric (#31891) [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973) SQL: Add support for single parameter text manipulating functions (#31874) [ML] Ensure immutability of MlMetadata (#31957) Tests: Mute SearchFieldsIT.testDocValueFields() muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) Tests: Remove use of joda time in some tests (#31922) [Test] Reactive 3rd party tests on CI (#31919) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) Add Snapshots Status API to High Level Rest Client (#31515) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Switch test framework to new style requests (#31939) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) [TEST] Mute SlackMessageTests.testTemplateRender Fix assertIngestDocument wrongfully passing (#31913) Remove unused reference to filePermissionsCache (#31923) rolling upgrade should use a replica to prevent relocations while running a scroll HLREST: Bundle the x-pack protocol project (#31904) Increase logging level for testStressMaybeFlush Added lenient flag for synonym token filter (#31484) [X-Pack] Beats centralized management: security role + licensing (#30520) HLRest: Move xPackInfo() to xPack().info() (#31905) Docs: add security delete role to api call table (#31907) [test] port archive distribution packaging tests (#31314) Watcher: Slack message empty text (#31596) [ML] Mute failing DetectionRulesIT.testCondition() test Fix broken NaN check in MovingFunctions#stdDev() (#31888) Date: Add DateFormatters class that uses java.time (#31856) [ML] Switch native QA tests to a 3 node cluster (#31757) Change trappy float comparison (#31889) Fix building AD URL from domain name (#31849) Add opaque_id to audit logging (#31878) re-enable backcompat tests add support for is_write_index in put-alias body parsing (#31674) Improve release notes script (#31833) [DOCS] Fix broken link in painless example Handle missing values in painless (#30975) Remove the ability to index or query context suggestions without context (#31007) Ingest: Enable Templated Fieldnames in Rename (#31690) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Ingest: Add ignore_missing option to RemoveProc (#31693) Add template config for Beat state to X-Pack Monitoring (#31809) Watcher: Add ssl.trust email account setting (#31684) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879) HLREST: Add x-pack-info API (#31870)

* 6.x: Force execution of fetch tasks (#31974) [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [test] disable java packaging tests for suse XContentTests : Insert random fields at random positions (#30867) Add Get Snapshots High Level REST API (#31980) Fix unreachable error condition in AmazonS3Fixture (#32005) [6.x][ML] Ensure immutability of MlMetadata (#31994) Add Expected Reciprocal Rank metric (#31891) SQL: Add support for single parameter text manipulating functions (#31874) muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) [Test] Reactive 3rd party tests on CI (#31919) Fix assertIngestDocument wrongfully passing (#31913) (#31951) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Switch test framework to new style requests (#31939) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Watcher: Slack message empty text (#31596) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) HLREST: Bundle the x-pack protocol project (#31904) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) Increase logging level for testStressMaybeFlush rolling upgrade should use a replica to prevent relocations while running a scroll [test] port archive distribution packaging tests (#31314) HLRest: Move xPackInfo() to xPack().info() (#31905) Increase logging level for :qa:rolling-upgrade Backport: Add template config for Beat state to X-Pack Monitoring (#31809) (#31893) Fix building AD URL from domain name (#31849) Fix broken NaN check in MovingFunctions#stdDev() (#31888) Change trappy float comparison (#31889) Add opaque_id to audit logging (#31878) add support for is_write_index in put-alias body parsing (#31674) Ingest: Enable Templated Fieldnames in Rename (#31690) (#31896) Ingest: Add ignore_missing option to RemoveProc (#31693) (#31892) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Watcher: Add ssl.trust email account setting (#31684) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) HLREST: Add x-pack-info API (#31870) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879)

andyb-elastic added >test Issues or PRs that are addressing/adding tests :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v7.0.0 v6.4.0 labels Jun 14, 2018

andyb-elastic requested a review from nik9000 June 14, 2018 02:32

andyb-elastic force-pushed the feature-java-archive-tests-2 branch from 35386a6 to e295e47 Compare June 14, 2018 17:36

andyb-elastic force-pushed the feature-java-archive-tests-2 branch from e295e47 to 1e1bc67 Compare June 14, 2018 18:02

nik9000 reviewed Jun 18, 2018

View reviewed changes

alpar-t reviewed Jun 25, 2018

View reviewed changes

andyb-elastic added 4 commits July 3, 2018 11:04

Merge branch 'master' into feature-java-archive-tests-2

67c73e2

review: no delay in waiting for server

17291fa

review: change platform conditionals to helper methods

2686d60

review: abstract which executables are chosen by platform

45dff8a

alpar-t reviewed Jul 4, 2018

View reviewed changes

code review fixes

8012b78

alpar-t approved these changes Jul 6, 2018

View reviewed changes

Merge branch 'master' into feature-java-archive-tests-2

25ce2ef

rjernst approved these changes Jul 9, 2018

View reviewed changes

andyb-elastic merged commit 006c2c9 into elastic:master Jul 10, 2018

andyb-elastic added the backport pending label Jul 10, 2018

andyb-elastic mentioned this pull request Jul 10, 2018

backport port archive distribution packaging tests to 6.x #31932

Merged

andyb-elastic mentioned this pull request Jul 17, 2018

java packaging tests #32143

Closed

24 tasks

danielmitterdorfer removed the backport pending label Jan 31, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

[test] port archive distribution packaging tests #31314

[test] port archive distribution packaging tests #31314

Conversation

andyb-elastic commented Jun 14, 2018

elasticmachine commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented Jul 4, 2018

alpar-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented Jul 5, 2018

alpar-t commented Jul 6, 2018

alpar-t commented Jul 6, 2018

rjernst left a comment

Choose a reason for hiding this comment

andyb-elastic commented Jul 9, 2018

andyb-elastic Jul 3, 2018 •

edited

Loading