Fix upgrade_through_versions_test.py::TestUpgrade* tests #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Run generated upgrade_through_versions_test on pytest >7.2.0 pytest-7.2.0 changed how markers were inherited, pytest-dev/pytest#7792 Remove how internode_ssl was changing seeds to append the ssl storage port, it's not needed as the tests always already set enable_legacy_ssl_storage_port to true. patch by Mick Semb Wever; reviewed by Brandon Williams for CASSANDRA-18499
… on the current jdk"
| @@ -325,6 +326,8 @@ def fixture_add_additional_log_patterns(self, fixture_dtest_setup): | |||
| ) | |||
|
|
|||
| def prepare(self): | |||
| if type(self).__name__ == "TestUpgrade": | |||
There was a problem hiding this comment.
I like this change
|
|
||
| def current_env_java_version(): | ||
| java_command = os.path.join(os.environ['JAVA_HOME'], 'bin', 'java') | ||
| output = subprocess.check_output([java_command, '-version'], stderr=subprocess.STDOUT).decode('utf-8') |
There was a problem hiding this comment.
this is already implemented:
from ccmlib.common import get_jdk_version_int
return get_jdk_version_int(java_command)
|
|
||
|
|
||
| def current_env_java_version(): | ||
| java_command = os.path.join(os.environ['JAVA_HOME'], 'bin', 'java') |
There was a problem hiding this comment.
please fail somewhere with a clear message when JAVA_HOME is not defined
There was a problem hiding this comment.
not needed, see update.
| # replace matching meta with current version | ||
| for idx, meta in enumerate(metas): | ||
| if meta.matches_current_env_version_family: | ||
| java_version = current_env_java_version() |
There was a problem hiding this comment.
we should get that java version once and do not do that in every iteration in a loop
There was a problem hiding this comment.
it should only called once, i was missing a break
michaelsembwever
force-pushed
the
mck/CASSANDRA-18499
branch
from
27f7af5
to
fcd0294
Compare
| create_upgrade_class(upgrade.name, [m for m in metas], protocol_version=upgrade.protocol_version, extra_config=upgrade.extra_config) | ||
| else: | ||
| # even for RUN_STATIC_UPGRADE_MATRIX we only test upgrade paths jdk compatible with the end "indev_" version (or any JAVA<jdk_version>_HOME defined) | ||
| metas = jdk_compatible_steps(upgrade.version_metas) |
There was a problem hiding this comment.
you are still calling current_env_java_version() in a loop
There was a problem hiding this comment.
oh, your talking about the outer loop for upgrade in MULTI_UPGRADES:
jacek-lewandowski
approved these changes
Jul 20, 2023
michaelsembwever
force-pushed
the
mck/CASSANDRA-18499
branch
from
fcd0294
to
b841171
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Run generated upgrade_through_versions_test on pytest >7.2.0
pytest-7.2.0 changed how markers were inherited, pytest-dev/pytest#7792
Remove how internode_ssl was changing seeds to append the ssl storage port, it's not needed as the tests always already set enable_legacy_ssl_storage_port to true.