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.

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

Update CcmBridge#getCurrentJvmMajorVersion to use regular expressions #1738

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.commons.exec.CommandLine;
import org.apache.commons.exec.DefaultExecutor;
Expand All @@ -47,6 +49,7 @@
import org.apache.commons.exec.LogOutputStream;
import org.apache.commons.exec.PumpStreamHandler;
import org.assertj.core.util.Lists;
import org.assertj.core.util.VisibleForTesting;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -111,6 +114,9 @@ public class CcmBridge implements AutoCloseable {
private static final Version V3_0_15 = Version.parse("3.0.15");
private static final Version V2_1_19 = Version.parse("2.1.19");

private static final Pattern JVM_VERSION_PATTERN =
Pattern.compile("^(1\\.)?(?<major>[0-9]+)\\..*$");

static {
if (DSE_ENABLEMENT) {
LOG.info("CCM Bridge configured with DSE version {}", VERSION);
Expand Down Expand Up @@ -430,17 +436,18 @@ private static File createTempStore(String storePath) {
*
* @return major version of current JVM
*/
private static int getCurrentJvmMajorVersion() {
String version = System.getProperty("java.version");
if (version.startsWith("1.")) {
version = version.substring(2, 3);
} else {
int dot = version.indexOf(".");
if (dot != -1) {
version = version.substring(0, dot);
}
@VisibleForTesting
static int getCurrentJvmMajorVersion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'm kinda wondering if instead of making this visible for testing we don't just change things so that all the logic lives in a getCurrentJvmMajorVersion() impl which takes a version string as an arg:

private static int getCurrentJvmMajorVersion() {
   return getJvmMajorVersion(System.getProperty("java.version"));
}

static int getJvmMajorVersion(String versionSysprop) {
   // Same logic in here (without the sysprop access)
}

Would probably simplify your tests quite a bit.

return getJvmMajorVersion(System.getProperty("java.version"));
}

@VisibleForTesting
static int getJvmMajorVersion(String version) {
Matcher matcher = version != null ? JVM_VERSION_PATTERN.matcher(version) : null;
if (matcher == null || !matcher.matches() || matcher.group("major") == null) {
throw new IllegalStateException("Unable to parse JVM version: " + version);
}
return Integer.parseInt(version);
return Integer.parseInt(matcher.group("major"));
}

private Optional<Integer> overrideJvmVersionForDseWorkloads() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.datastax.oss.driver.api.testinfra.ccm;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

import org.junit.Test;

public class CcmBridgeTest {

@Test
public void should_parse_system_jvm() {
assertThat(CcmBridge.getCurrentJvmMajorVersion()).isGreaterThan(0);
}

@Test
public void should_parse_jvm_8_like() {
assertThat(CcmBridge.getJvmMajorVersion("1.8.0_181")).isEqualTo(8);
}

@Test
public void should_parse_jvm_8_like_with_trailing_alphabetic() {
assertThat(CcmBridge.getJvmMajorVersion("1.8.0_181_b1")).isEqualTo(8);
}

@Test
public void should_parse_jvm_11_like() {
assertThat(CcmBridge.getJvmMajorVersion("11.0.20.1")).isEqualTo(11);
}

@Test
public void should_parse_jvm_17_like() {
assertThat(CcmBridge.getJvmMajorVersion("17.0.8.1")).isEqualTo(17);
}

@Test
public void should_parse_jvm_21_like() {
assertThat(CcmBridge.getJvmMajorVersion("21.0.1")).isEqualTo(21);
}

@Test
public void should_fail_null_version() {
assertThatCode(() -> CcmBridge.getJvmMajorVersion(null))
.isInstanceOf(IllegalStateException.class);
}

@Test
public void should_fail_empty_version() {
assertThatCode(() -> CcmBridge.getJvmMajorVersion(""))
.isInstanceOf(IllegalStateException.class);
}

@Test
public void should_fail_non_number() {
assertThatCode(() -> CcmBridge.getJvmMajorVersion("abc"))
.isInstanceOf(IllegalStateException.class);
}

@Test
public void should_fail_not_versioned() {
assertThatCode(() -> CcmBridge.getJvmMajorVersion("8"))
.isInstanceOf(IllegalStateException.class);
}
}