-
Notifications
You must be signed in to change notification settings - Fork 313
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.
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
Validate if car allows for using the bundled JDK #987
Changes from 7 commits
5e297ae
0c0a892
76d6528
7255c99
e95be18
c30eef8
20eb79f
d422648
ac3e578
667fa79
451e2cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ | |
import unittest.mock as mock | ||
from unittest import TestCase | ||
|
||
from esrally import config | ||
from esrally import config, exceptions | ||
from esrally.mechanic import java_resolver | ||
|
||
|
||
|
@@ -30,7 +30,9 @@ def test_resolves_java_home_for_default_runtime_jdk(self, resolve_jvm_path): | |
cfg = config.Config() | ||
cfg.add(config.Scope.application, "mechanic", "runtime.jdk", None) | ||
|
||
major, java_home = java_resolver.java_home("12,11,10,9,8", cfg) | ||
major, java_home = java_resolver.java_home("12,11,10,9,8", | ||
specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need for the |
||
provides_bundled_jdk=True) | ||
|
||
self.assertEqual(major, 12) | ||
self.assertEqual(java_home, "/opt/jdk12") | ||
|
@@ -42,7 +44,9 @@ def test_resolves_java_home_for_specific_runtime_jdk(self, resolve_jvm_path): | |
cfg = config.Config() | ||
cfg.add(config.Scope.application, "mechanic", "runtime.jdk", 8) | ||
|
||
major, java_home = java_resolver.java_home("12,11,10,9,8", cfg) | ||
major, java_home = java_resolver.java_home("12,11,10,9,8", | ||
specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need for the |
||
provides_bundled_jdk=True) | ||
|
||
self.assertEqual(major, 8) | ||
self.assertEqual(java_home, "/opt/jdk8") | ||
|
@@ -53,9 +57,22 @@ def test_resolves_java_home_for_bundled_jdk(self): | |
cfg = config.Config() | ||
cfg.add(config.Scope.application, "mechanic", "runtime.jdk", "bundled") | ||
|
||
major, java_home = java_resolver.java_home("12,11,10,9,8", cfg) | ||
major, java_home = java_resolver.java_home("12,11,10,9,8", | ||
specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need for the |
||
provides_bundled_jdk=True) | ||
|
||
# assumes most recent JDK | ||
self.assertEqual(major, 12) | ||
# does not set JAVA_HOME for the bundled JDK | ||
self.assertEqual(java_home, None) | ||
|
||
def test_disallowed_bundled_jdk(self): | ||
|
||
cfg = config.Config() | ||
cfg.add(config.Scope.application, "mechanic", "runtime.jdk", "bundled") | ||
cfg.add(config.Scope.application, "mechanic", "car.names", ["default"]) | ||
|
||
with self.assertRaises(exceptions.SystemSetupError) as ctx: | ||
java_resolver.java_home("12,11,10,9,8", specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need for the |
||
self.assertEqual("This Elasticsearch version does not contain a bundled JDK. Please specify a different runtime JDK.", | ||
ctx.exception.args[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the conversion (i.e.
convert.to_bool
to the call site(s)?) IMHO we should use the correct typebool
already in the constructor.