Skip to content

Migrate systemd packaging tests from bats to java#39954

Merged
pgomulka merged 18 commits intoelastic:masterfrom
pgomulka:feature/packaging-tests
Mar 28, 2019
Merged

Migrate systemd packaging tests from bats to java#39954
pgomulka merged 18 commits intoelastic:masterfrom
pgomulka:feature/packaging-tests

Conversation

@pgomulka
Copy link
Copy Markdown
Contributor

@pgomulka pgomulka commented Mar 12, 2019 •
edited
Loading

Migrating systemd bats tests from bats to java dsl. This also covers partially the sysv, but more must be added

relates #32143

@pgomulka pgomulka added >test Issues or PRs that are addressing/adding tests :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts WIP labels Mar 12, 2019
@pgomulka pgomulka self-assigned this Mar 12, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 12, 2019

Pinging @elastic/es-core-infra

@andyb-elastic andyb-elastic mentioned this pull request Mar 12, 2019
Closed
24 tasks
@pgomulka pgomulka force-pushed the feature/packaging-tests branch from 80fb18a to 48bfbaf Compare March 12, 2019 13:29
@pgomulka pgomulka changed the title WIP migrate systemd and sysv from bats to java WIP migrate systemd and sysv packaging tests from bats to java Mar 12, 2019
@pgomulka
Copy link
Copy Markdown
Contributor Author

pgomulka commented Mar 21, 2019

This is now ready for a review. The systemd part has been migrated.

@pgomulka pgomulka removed the WIP label Mar 21, 2019
@pgomulka pgomulka changed the title WIP migrate systemd and sysv packaging tests from bats to java WIP migrate systemd packaging tests from bats to java Mar 21, 2019
@pgomulka pgomulka changed the title WIP migrate systemd packaging tests from bats to java Migrate systemd packaging tests from bats to java Mar 21, 2019
@pgomulka
Copy link
Copy Markdown
Contributor Author

pgomulka commented Mar 22, 2019

@elasticmachine run elasticsearch-ci/packaging

alpar-t
alpar-t reviewed Mar 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @pgomulka I love to see the bats tests go !
I left a few comments and questions.

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java
stopElasticsearch(newShell());
}

// TEST CASES FOR SYSTEMD ONLY
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

In the interest of keeping this class from growing, could we have a separate class fro the systemd tests ?

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 22, 2019

Choose a reason for hiding this comment

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

There is 8 subclasses of this testcase now, if we separate systemd from sysv we will endup with 16. Most of the time we share the logic for testing between systemd and sysv. I think this is ok for now, but definitely we should consider restructuring the hierarchy once this grow too big.

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java Outdated
installation = install(distribution());
assertInstalled(distribution());

Shell sh = newShell();
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

would it make sense to have this as a field set up in a @Before ?

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 22, 2019

Choose a reason for hiding this comment

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

good idea, this is repeated all over the testcases

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java Outdated


public void test72TestRuntimeDirectory() throws IOException {
cleanup();
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

could this be done in @After or @Before if you wish to avoid the repetitive calls and make sure we always start clean ? if we have tests that break with this now, I think that's something we need to fix as it makes the test hard to read if different tests depend on each-other even when run in sequence.

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 22, 2019

Choose a reason for hiding this comment

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

right, I think not all tests are leaving the environment dirty. Might be better to keep cleanup in the test that is making the changes to environment in try-finally block

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java Outdated
public void test72TestRuntimeDirectory() throws IOException {
cleanup();
installation = install(distribution());
FileUtils.rm(installation.pidDir);
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

nit: static imports of the methods in FileUtils would make this slightly more readbale

qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java Outdated
installation = install(distribution());
startElasticsearch(newShell());
// it can be gc.log or gc.log.0.current
assertThat(installation.logs, FileUtils.fileWithRegexExist("gc.log*"));
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

I think technically that's a glob not a regexp.

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 22, 2019

Choose a reason for hiding this comment

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

right, good spot

qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Packages.java Outdated

waitForElasticsearch();

// assertStatuses();
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

is this intentionally commented out ?

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 22, 2019

Choose a reason for hiding this comment

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

I had problems with running status after restart. Will have a second look but for time being will remove it.

qa/vagrant/src/test/resources/packaging/tests/60_systemd.bats
# Rather than restart the VM we just ask systemd if it plans on starting
# elasticsearch on restart. Not as strong as a restart but much much
# faster.
run systemctl is-enabled elasticsearch.service
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

Did we miss porting this check ?

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 26, 2019

Choose a reason for hiding this comment

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

yes I missed this one. added

qa/vagrant/src/test/resources/packaging/tests/60_systemd.bats
systemctl start elasticsearch.service
wait_for_elasticsearch_status
assert_file_exist "/var/run/elasticsearch/elasticsearch.pid"
assert_file_exist "/var/log/elasticsearch/elasticsearch_server.json"
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

Do we check this in the java version ?

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

good spot, it was not checked at all for packages tests

qa/vagrant/src/test/resources/packaging/tests/60_systemd.bats
result="$(journalctl _SYSTEMD_UNIT=elasticsearch.service --since "$since" --output cat | wc -l)"
[ "$result" -eq "0" ] || {
echo "Expected no entries in journalctl for the Elasticsearch service but found:"
journalctl _SYSTEMD_UNIT=elasticsearch.service --since "$since"
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

I think we are also missing this one.

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 27, 2019

Choose a reason for hiding this comment

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

Added. I wrongly assumed that since start is already migrated that I didn't have to look into this step :)

qa/vagrant/src/test/resources/packaging/tests/60_systemd.bats
@test "[SYSTEMD] test runtime directory" {
clean_before_test
install_package
sudo rm -rf /var/run/elasticsearch
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t Mar 22, 2019

Choose a reason for hiding this comment

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

might be missing this one too ?

Copy link
Copy Markdown
Contributor Author

@pgomulka pgomulka Mar 26, 2019

Choose a reason for hiding this comment

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

should be now in test72TestRuntimeDirectory

@Conky5
Copy link
Copy Markdown

Conky5 commented Mar 22, 2019

The last packaging build was impacted by https://github.com/elastic/infra/issues/10287.

@elasticmachine run elasticsearch-ci/packaging

@pgomulka pgomulka requested a review from alpar-t March 27, 2019 13:28
alpar-t
alpar-t approved these changes Mar 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

LGTM

@alpar-t
Copy link
Copy Markdown
Contributor

alpar-t commented Mar 28, 2019

@pgomulka the PR doesn't have any version labels. I do think this should be back-ported to 7.x

@pgomulka pgomulka merged commit 5ceef9e into elastic:master Mar 28, 2019
jasontedor added a commit to rjernst/elasticsearch that referenced this pull request Mar 28, 2019
@jasontedor
Merge branch 'master' into pr/40576
828ac34 
* master: (25 commits)
  [DOCS] Correct keystore commands for Email and Jira actions in Watcher (elastic#40417)
  [DOCS] Document common settings for snapshot repository plugins (elastic#40475)
  Remove with(out)-system-key tests (elastic#40547)
  Geo Point parse error fix (elastic#40447)
  Handle null retention leases in WaitForNoFollowersStep (elastic#40477)
  [DOCS] Adds anchors for ruby client (elastic#39867)
  Mute DataFrameAuditorIT#testAuditorWritesAudits
  Disable integTest when Docker is not available (elastic#40585)
  Add randomScore function in script_score query (elastic#40186)
  Test fixtures krb5 (elastic#40297)
  Correct ILM metadata minimum compatibility version (elastic#40569)
  SQL: Centralize SQL test dependencies version handling (elastic#40551)
  Mute testTracerLog
  Mute testHttpInput
  Include functions' aliases in the list of functions (elastic#40584)
  Optimise rejection of out-of-range `long` values (elastic#40325)
  Add docs for cluster.remote.*.proxy setting (elastic#40281)
  Migrate systemd packaging tests from bats to java (elastic#39954)
  Move top-level pipeline aggs out of QuerySearchResult (elastic#40319)
  Use openjdk 12 in packer cache script (elastic#40498)
  ...
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Apr 3, 2019
@pgomulka
Migrate systemd packaging tests from bats to java backport(elastic#39954
0d6ecce 
)

Migrating systemd bats tests from bats to java dsl. This also covers
partially the sysv, but more must be added

relates elastic#32143
@pgomulka pgomulka mentioned this pull request Apr 3, 2019
Merged
pgomulka added a commit that referenced this pull request Apr 4, 2019
@pgomulka
Migrate systemd packaging tests from bats to java backport(#39954) (#…
a6faf85 
…40763)

Migrating systemd bats tests from bats to java dsl. This also covers
partially the sysv, but more must be added

relates #32143
backport #39954
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Apr 4, 2019
@pgomulka
Migrate systemd packaging tests from bats to java backport(elastic#39954
6f89742 
)

Migrating systemd bats tests from bats to java dsl. This also covers
partially the sysv, but more must be added

relates elastic#32143
@pgomulka pgomulka mentioned this pull request Apr 4, 2019
Merged
pgomulka added a commit that referenced this pull request Apr 4, 2019
@pgomulka
Bat scripts to work with JAVA_HOME with parentheses Backports (#40770)
e15e337 
backports :
Migrate systemd packaging tests from bats to java (#39954)
Bat scripts to work with JAVA_HOME with parentheses (#39712)
Link to SYSTEM_JAVA_HOME on windows (#40806)
@jakelandis jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019
@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Jul 5, 2019

@pgomulka I'm assuming there is nothing left to backport and removed the backport pending label.

@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alpar-t alpar-t alpar-t approved these changes

Assignees

No one assigned

Labels

:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v7.3.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pgomulka @elasticmachine @Conky5 @alpar-t @jpountz @jakelandis @mark-vieira