Skip to content
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

Jump to bottom

Migrate systemd packaging tests from bats to java #39954

Merged
merged 18 commits into from
Mar 28, 2019
Merged

Migrate systemd packaging tests from bats to java #39954

merged 18 commits into from
Mar 28, 2019

Conversation

pgomulka
Copy link
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
Collaborator

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
Contributor Author

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
Contributor Author

@elasticmachine run elasticsearch-ci/packaging

alpar-t
alpar-t reviewed Mar 22, 2019
View reviewed changes
Copy link
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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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

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