[test] java tests for archive packaging

[andyb-elastic]

Ports the first couple tests for archive distributions from the old bats
project to the new java project that includes windows platforms,
consolidating them into one test method that tests that the
distributions can be extracted and their contents verified. Includes the
zip distributions which were not tested in the bats project.

For #26741

This takes into account @rjernst's changes in #30596

[elasticmachine]

Pinging @elastic/es-core-infra

[andyb-elastic]

Originally I used parameterized tests to serve this purpose but it seemed like this was a more easily understandable way to achieve the same thing

[andyb-elastic]

I'll make a follow up changing the bats tests to expect it in this location

[nik9000]

I wonder if this kind of thing should be moved to ArchiveTestCase and you subclass it for each time.

[nik9000]

Like you move the entire installArchive method to the abstract superclass.

[andyb-elastic]

Do you mean this kind of thing

// ArchiveTestCase.java

abstract void extractArchive(Path install);

Installation installArchive(Distribution distribution) {
  // first half of Archives#installArchive
  extractArchive(installLocation, distributionFile);
  // rest of Archives#installArchive
}

// DefaultTarTests.java
// ditto for OssTarTests.java

@Override
void extractArchive(Path install, Path distributionFile) {
  // run the untar command
}

I feel like there's some value from not coupling the install logic to the test class too tightly (although maybe that's a misplaced idea). Maybe something like this?

// ArchiveTestCase.java

abstract void extractArchive(Path install, Path distributionFile);

@Test
public void test10Install() {
  installation = Archives.installArchive(distribution, this::extractArchive);
  Archives.verify(installation, distribution());
}

// Archives.java

public Installation installArchive(Distribution d, BiConsumer<Path, Path> extractor) {
  // first half of Archives.installArchive
  extractor.accept(installPath, distributionFile);
  // rest of Archives.installArchive
}

// DefaultTarTests.java
// ditto for OssTarTests.java

@Override
void extractArchive(Path install, Path distributionFile) {
  // run the untar command
}

That said, the reason I ended up doing it this way was that I figured it was easier to follow if all this logic was in one place, even if all the if-branching was kind of gross, because there's only two archive types (and it seems unlikely there would ever be significantly more than that). What do you think?

[nik9000]

I think it is fine to do it as you are doing it. We could change it to my way later on if we figure it is better. I don't think it is a blocker.

I just look at your way and think "this made a ton of sense in bash scripts but I want to OO all the things in Java because I'm a Java developer." I admit that is a bad habit some times. I don't think it'd be too weird to keep the install logic in the superclass but you certainly are right that there aren't that many variants.

[andyb-elastic]

this made a ton of sense in bash scripts but I want to OO all the things in Java because I'm a Java developer

Yeah this was exactly what I did at first and then walked it back to here since it seemed simpler :)

I'll take a look at the other places in Archives' methods where this kind of stuff happens and see if I can find a happy medium

[andyb-elastic]

I tried a couple variations of what we discussed. I still feel like the benefit of being able to answer the question "how are the archive distros installed" by looking in one place outweighs that place being a little gross - not in general, but in this instance because we don't expect to have many, if any additional cases.

I agree that if it does change it would be better to do something like your suggestion. I'll also keep thinking about other ways to do it as I revise this

[nik9000]

This isn't passing for me:

Expected: "file/directory: "<File> with owner "elasticsearch" with group "elasticsearch" with posix permissions [<OWNER_READ>,<OWNER_WRITE>,<GROUP_READ>,<OTHERS_READ>]
     but: path </tmp/elasticsearch/NOTICE.txt>Has permissions [GROUP_WRITE, OWNER_READ, GROUP_READ, OWNER_WRITE, OTHERS_READ]

[andyb-elastic]

Weird, it passes locally for me and in CI. Which box and test?

[andyb-elastic]

Actually if you got a full reproduce line that would be best - I'm pretty sure the randomization doesn't affect anything about the current version installs, but best to check anyway

[nik9000]

I ran:

./gradlew -p qa/vagrant/ vagrantCentos7#javaPackagingTest

I'm pulling and trying again.

[nik9000]

Yeah. That command fails for me.

[andyb-elastic]

Weird, still not seeing it. If you still have it mind sharing the test seed it used

[nik9000]

I got this:

REPRODUCE WITH: ./gradlew :qa:vagrant:vagrantCentos7#javaPackagingTest -Dtests.seed=924BDF23E0249263 

Though I'm not sure how useful that is.

[jasontedor]

@andyb-elastic @nik9000 I recommend comparing umasks.

[rjernst]

This looks pretty good. I think it would be good to have some type of documentation on how to write/extend these tests, as there are a number of classes now that one must understand to change these tests. But that can be in followups.

[rjernst]

Aren't these all mutually exclusive?

[andyb-elastic]

They should be (at least deb vs rpm). That said I think the logic here does make sense (if any of these package managers exist on the system, remove the packages from them). It's the same in the bats tests

[rjernst]

I think it is far more often we want to assert success. Can we flip this around have have bare run assert, and have a runIgnoreFailure for the other case?

[andyb-elastic]

Yeah good point, it's also probably more intuitive that way

[andyb-elastic]

Talked to Nik and he said he's ✅ on this

[andyb-elastic]

@rjernst sure thing can do a follow up with some docs about writing tests for this project. Where's the right place for those to go? Readme in qa/vagrant?

[rjernst]

Where's the right place for those to go? Readme in qa/vagrant?

That's a good place to start.