#345: Replace ExitGuard in integration tests #438
Conversation
kaklakariada
changed the title
|
|
||
| import com.exasol.mavenprojectversiongetter.MavenProjectVersionGetter; | ||
|
|
||
| class JarLauncher |
There was a problem hiding this comment.
I think this class deserves a bit of documentation in the header. :-)
|
|
||
| public static JarLauncher start(final Path workingDir, final List<String> args) | ||
| { | ||
| final Path jarPath = getExecutableJar(); |
There was a problem hiding this comment.
| final Path jarPath = getExecutableJar(); | |
| final Path jarPath = getExecutableJarPath(); |
| } | ||
| } | ||
|
|
||
| private static Path getExecutableJar() |
There was a problem hiding this comment.
| private static Path getExecutableJar() | |
| private static Path getExecutableJarPath() |
| "Executable JAR not found at %s. Run 'mvn -T1C package -DskipTests' to build it."); | ||
| } | ||
| final List<String> command = new ArrayList<>(); | ||
| command.addAll(asList(getJavaExecutable().toString(), "-jar", getExecutableJar().toString())); |
There was a problem hiding this comment.
| command.addAll(asList(getJavaExecutable().toString(), "-jar", getExecutableJar().toString())); | |
| command.addAll(List.of(getJavaExecutable().toString(), "-jar", getExecutableJar().toString())); |
| { | ||
| return runnable -> { | ||
| final Thread thread = new Thread(runnable); | ||
| thread.setName(ProcessOutputConsumer.class.getSimpleName()); |
There was a problem hiding this comment.
Wouldn't it make more sense to name the thread after the process being consumed? Or at least to get that processes identification into the name? Thread names are mainly a debugging means as far as I remember.
| { | ||
| try (final BufferedReader reader = new BufferedReader(new InputStreamReader(stream, StandardCharsets.UTF_8))) | ||
| { | ||
| String line = null; |
There was a problem hiding this comment.
Strings are immutable. Does reusing it have any runtime benefit in this case? I would assume not, since the memory will be freshly allocated with each new assignment.
| return builder.toString(); | ||
| } | ||
|
|
||
| void streamFinished() |
There was a problem hiding this comment.
Naming an attribute and a method the same is a recipe for confusion.
Please rename the method. Use an imperative. :-)
|
|
||
| private static class ProcessStreamConsumer | ||
| { | ||
| private final CountDownLatch streamFinished = new CountDownLatch(1); |
There was a problem hiding this comment.
| private final CountDownLatch streamFinished = new CountDownLatch(1); | |
| private final CountDownLatch streamFinishedLatch = new CountDownLatch(1); |
|
|
||
| void accept(final String line) | ||
| { | ||
| LOG.fine("%s > %s".formatted(name, line)); |
There was a problem hiding this comment.
Consuming without logging might be a good option, since we are in tests here and don't want to flood the test log.
| if (!Files.exists(jarPath)) | ||
| { | ||
| throw new IllegalStateException( | ||
| "Executable JAR not found at %s. Run 'mvn -T1C package -DskipTests' to build it."); |
product/src/test/java/org/itsallcode/openfasttrace/cli/JarLauncher.java
Outdated
Show resolved
Hide resolved
| { | ||
| return Path.of("target") | ||
| .resolve(Objects.requireNonNull(jarNameTemplate, "jarNameTemplate") | ||
| .formatted(getCurrentProjectVersion())) |
There was a problem hiding this comment.
That is a tiny bit hard to read if you don't remember the %s in the template. But I can live with it.
| .orElseThrow(() -> new IllegalStateException("Java executable not found")); | ||
| } | ||
|
|
||
| public void waitUntilTerminated(final Duration timeout) |
There was a problem hiding this comment.
| public void waitUntilTerminated(final Duration timeout) | |
| public void assertExpectationsAfterTerminated(final Duration timeout) |
Closes #345.