Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Refactor runner, adding support for kotlin #129

Merged
merged 7 commits into from
Aug 19, 2022
Merged

Refactor runner, adding support for kotlin #129

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d85c48
feat: add kotlin runner test support
jamesreprise Aug 18, 2022
bda2a6c
empty commit to nudge ci
jamesreprise Aug 18, 2022
c797c5b
fix: move CLIOptions inside Util
jamesreprise Aug 18, 2022
bc649ac
fix: naming of classpath_resources was erroneously changed
jamesreprise Aug 18, 2022
5aa788b
fix: refactor runner bazel rules
jamesreprise Aug 18, 2022
1449a1b
fix: cleaned up names, dissolved FLAG argument into implicit functions
jamesreprise Aug 18, 2022
499a4b6
fix: native_dependencies typo
jamesreprise Aug 18, 2022
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
1 change: 1 addition & 0 deletions test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ java_library(
deps = [
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned previously, the CI for this repo is insufficient to verify correctness. Have we tested these changes? If so, how/where?

In general, I would say it's worth including this information in the PR description and/or comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have! All of typedb-client-java's tests run successfully with this version of typedb-common. Would it make sense to just say that this is the case or to also post the test output?

Copy link
Member

Choose a reason for hiding this comment

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

No need - that last comment "All of typedb-client-java's tests run successfully with this version of typedb-common" is enough by itself!

"//:common",
"@maven//:commons_io_commons_io",
"@maven//:info_picocli_picocli",
Copy link
Member

Choose a reason for hiding this comment

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

As a general rule, we should always write PR comments on new dependencies, especially when they come from a new "domain" that isn't already in the dependencies. For example, adding a new Compose sub-library when we already have 20 others is probably OK, but adding picocli (which is unrelated to the other deps on the current list) warrants a one-line explanation.

In this case, said explanation already exists in the PR description, so this is just a matter of copying it across to comments for faster access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I'll make sure to do this in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it now as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We leverage picocli to parse command line arguments without having to write string parsing ourselves, which is error-prone and likely to be more difficult to maintain and extend.

"@maven//:junit_junit",
"@maven//:org_zeroturnaround_zt_exec",
"@maven//:org_slf4j_slf4j_api"
Expand Down
61 changes: 53 additions & 8 deletions test/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.zeroturnaround.exec.ProcessExecutor;
import org.zeroturnaround.exec.ProcessResult;
import org.zeroturnaround.exec.StartedProcess;
import picocli.CommandLine;

import java.io.File;
import java.io.IOException;
Expand All @@ -36,12 +37,12 @@
import java.util.List;
import java.util.Timer;
import java.util.TimerTask;
import java.util.Optional;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import static org.junit.Assert.fail;

public class Util {

Expand All @@ -52,16 +53,20 @@ public class Util {
private static final int SERVER_ALIVE_POLL_INTERVAL_MILLIS = 500;
private static final int SERVER_ALIVE_POLL_MAX_RETRIES = SERVER_STARTUP_TIMEOUT_MILLIS / SERVER_ALIVE_POLL_INTERVAL_MILLIS;

public static File getArchivePath(int index) {
public static File getArchivePath(String flag) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use indices anymore, so we pass the flag that the resource is seeking and use that to find the path.

String[] args = System.getProperty("sun.java.command").split(" ");
if (index >= args.length) {
throw new IllegalArgumentException("Distribution archive at index '" + index + "' is not defined");
Optional<CLIOptions> maybeOptions = CLIOptions.parseCLIOptions(args);
Copy link
Member

Choose a reason for hiding this comment

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

Optional options, lovely. (maybe)

Copy link
Contributor Author

@jamesreprise jamesreprise Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's a nice pattern and makes the potential for an empty result explicit instead of jumping to null. Someone should make a language that uses a load of these everywhere...

if (!maybeOptions.isPresent()) {
throw new IllegalArgumentException("No archives were passed as arguments");
}
File file = new File(args[index]);
if (!file.exists()) {
throw new IllegalArgumentException("Distribution archive '" + file.getAbsolutePath() + "' is missing");
CLIOptions options = maybeOptions.get();
if (flag.equals("--server")) {
return new File(options.getServerArchive());
}
return file;
if (flag.equals("--console")) {
return new File(options.getConsoleArchive());
}
throw new IllegalArgumentException("Unrecognised arguments");
}

public static Path unarchive(File archive) throws IOException, TimeoutException, InterruptedException {
Expand Down Expand Up @@ -199,4 +204,44 @@ public static ProcessExecutor createProcessExecutor(Path directory) {
.readOutput(true)
.destroyOnExit();
}

@CommandLine.Command(name = "java")
private static class CLIOptions {
@CommandLine.Parameters String mainClass;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class that is being run during execution, e.g. com.google.testing.junit.runner.BazelTestRunner. We need to capture this in the command otherwise picocli thinks it has been passed an unrecognised parameter.

@CommandLine.Option(
names = {"--server"},
description = "Location of the archive containing a server artifact."
)
private String serverArchive;

@CommandLine.Option(
names = {"--console"},
description = "Location of the archive containing a console artifact."
)
private String consoleArchive;

public String getServerArchive() {
return serverArchive;
}

public String getConsoleArchive() {
return consoleArchive;
}

public static Optional<CLIOptions> parseCLIOptions(String[] args) {
CommandLine commandLine = new CommandLine(new CLIOptions());
try {
CommandLine.ParseResult result = commandLine.parseArgs(args);
return Optional.of(result.asCommandLineList().get(0).getCommand());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the parse result succeeded, we get the first command in the commandLineList and return it in an optional.

} catch (CommandLine.ParameterException ex) {
commandLine.getErr().println(ex.getMessage());
if (!CommandLine.UnmatchedArgumentException.printSuggestions(ex, commandLine.getErr())) {
ex.getCommandLine().usage(commandLine.getErr());
}
return Optional.empty();
}
}
}
}


5 changes: 2 additions & 3 deletions test/cluster/TypeDBClusterServerRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,14 @@ protected TypeDBClusterServerRunner createServerRunner(Map<String, String> optio

class Standalone implements TypeDBClusterServerRunner {

private static final int ARCHIVE_INDEX = 1;

private static final String FLAG = "--server";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cluster is the server, we don't differentiate between cluster and core because we wouldn't run both at once as they share the same purpose.

Copy link
Member

Choose a reason for hiding this comment

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

FLAG isn't a particularly meaningful name here. I would consider ARCHIVE_PATH_PARAM, but there's another issue just below...

This file really had quite a bit of black magic in it, didn't it? We need to dispel it where possible. On line 64 we do

distribution = unarchive(getArchivePath(FLAG));

Even after renaming FLAG, it's still not at all clear what that line will do. We need to rethink this.

One possible band-aid would be renaming FLAG to SERVER. That makes line 64's intent clearer, but it dissolves the clarity of line 57. So the issue must run a little deeper.

As another perspective: given that there are only ever two kinds of archives, server and console, how about introducing methods named getServerArchivePath and getConsoleArchivePath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I went with getServerArchiveFile to make it more explicit that we're returning a File, not a Path.

protected final Path distribution;
protected final Map<String, String> serverOptions;
private StartedProcess process;
protected ProcessExecutor executor;

public Standalone(Map<String, String> serverOptions) throws IOException, InterruptedException, TimeoutException {
distribution = unarchive(getArchivePath(ARCHIVE_INDEX));
distribution = unarchive(getArchivePath(FLAG));
this.serverOptions = serverOptions;
System.out.println(addresses() + ": " + name() + " constructing runner...");
Files.createDirectories(ClusterServerOpts.storageData(serverOptions));
Expand Down
5 changes: 2 additions & 3 deletions test/console/TypeDBConsoleRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,14 @@

public class TypeDBConsoleRunner {

private static final int ARCHIVE_INDEX = 2;

private static final String FLAG = "--console";
protected final Path distribution;
protected ProcessExecutor executor;

public TypeDBConsoleRunner() throws InterruptedException, TimeoutException, IOException {
System.out.println("Constructing " + name() + " runner");
System.out.println("Extracting " + name() + " distribution archive.");
distribution = unarchive(getArchivePath(ARCHIVE_INDEX));
distribution = unarchive(getArchivePath(FLAG));
System.out.println(name() + " distribution archive extracted.");
executor = new ProcessExecutor()
.directory(distribution.toFile())
Expand Down
4 changes: 2 additions & 2 deletions test/core/TypeDBCoreRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

public class TypeDBCoreRunner implements TypeDBRunner {

private static final int ARCHIVE_INDEX = 1;
private static final String FLAG = "--server";

private final Path distribution;
private final Path dataDir;
Expand All @@ -50,7 +50,7 @@ public TypeDBCoreRunner() throws InterruptedException, TimeoutException, IOExcep
port = findUnusedPorts(1).get(0);
System.out.println(address() + ": Constructing " + name() + " runner");
System.out.println(address() + ": Extracting distribution archive...");
distribution = unarchive(getArchivePath(ARCHIVE_INDEX));
distribution = unarchive(getArchivePath(FLAG));
System.out.println(address() + ": Distribution archive extracted.");
dataDir = distribution.resolve("server").resolve("data");
logsDir = distribution.resolve("server").resolve("logs");
Expand Down
56 changes: 47 additions & 9 deletions test/rules.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,68 @@
#

load("@vaticle_dependencies//builder/java:rules.bzl", "native_dep_for_host_platform")
load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_test")

def typedb_java_test(name, server_mac_artifact, server_linux_artifact, server_windows_artifact,
console_mac_artifact = None, console_linux_artifact = None, console_windows_artifact = None,
native_libraries_deps = [], deps = [], classpath_resources = [], data = [], args = [], **kwargs):

native_server_artifact_paths, native_server_artifact_labels = native_artifact_paths_and_labels(
server_mac_artifact, server_linux_artifact, server_windows_artifact
)
native_console_artifact_paths, native_console_artifact_labels = [], []
if console_mac_artifact and console_linux_artifact and console_windows_artifact:
native_console_artifact_paths, native_console_artifact_labels = native_artifact_paths_and_labels(
server_mac_artifact, server_linux_artifact, server_windows_artifact
)

native_console_artifact_paths, native_console_artifact_labels = native_console_artifact_paths_and_labels(
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of removing extraneous code from the top-level function typedb_java_test. However, my first thought when seeing two methods: native_console_artifact_paths_and_labels, and native_artifact_paths_and_labels is: "shouldn't these do the same thing?"

Upon investigation, they do appear to do the same thing - except that native_console_artifact_paths_and_labels additionally verifies that the three artifacts are actually defined (i.e: non-null). Is this correct?

If so, I think the thing to improve here is: "check Y exists, then do X" is quite a thin abstraction over "do X", and we should delete the new method, and then, could either:

  • Make native_artifact_paths_and_labels always check that the artifacts exist first, or:
  • Add a boolean parameter to native_artifact_paths_and_labels, called, say, mandatory (there may be a better name). If the artifacts don't exist and mandatory is False, return empty lists; if they don't exist and mandatory is True, throw an error using fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks much nicer. Done.

console_mac_artifact, console_linux_artifact, console_windows_artifact
)
native_deps = []
for dep in native_libraries_deps:
native_deps = native_deps + native_dep_for_host_platform(dep)

native_deps = add_native_libraries_deps(native_libraries_deps)
Copy link
Member

Choose a reason for hiding this comment

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

As we would be strict on naming conventions in Rust, so should we be in Starlark. add_xxx means "take an existing collection and add item(s) to it; what we're doing here is constructing a new collection, so the name of the method should just be native_libraries_deps, not add_native_libraries_deps.

Of course, that conflicts with the parameter name, however...

I'm also not comfortable with the fact that native_deps and native_libraries_deps sound like they mean the same thing, but they are actually different. native_libraries_deps are "specifications of what platform-specific dependencies we should include depending on our target platform." native_deps are "platform-specific dependencies for the current platform."

(This is not technical debt introduced in this PR, this has been around before - but now that we're working on this file, we should address this issue)

Copy link
Contributor Author

@jamesreprise jamesreprise Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, this was poorly named. We're taking the passed native_libraries_deps and turning them into the dependencies that they specify.

Changing the name native_libraries_deps in the method signature of typedb_java_test could be perilous, see https://github.com/search?p=1&q=org%3Avaticle+native_libraries_deps&type=Code

As such, I've gone with

native_dependencies = get_native_dependencies(native_libraries_deps)

I read this as 'we are getting the native dependencies using the specifications of the platform-specific dependencies we need for our target platform'.

This is a pretty thorny one, so let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think we may need to address the heart of this problem, which is that native_libraries_deps itself is an awkward, ambiguous name. This will be a fairly wide-reaching refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would renaming the variable immediately work as a stop-gap, or should we leave it as is?

I've raised an issue for this:

Copy link
Member

Choose a reason for hiding this comment

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

Let's stick with the imperfect name and merge this PR. Finding a perfect name would be impossible in an imperfect environment.


native.java_test(
name = name,
deps = depset(deps + ["@vaticle_typedb_common//test:typedb-runner"]).to_list() + native_deps,
classpath_resources = depset(classpath_resources + ["@vaticle_typedb_common//test:logback"]).to_list(),
data = data + select(native_server_artifact_labels) + (select(native_console_artifact_labels) if native_console_artifact_labels else []),
args = select(native_server_artifact_paths) + (select(native_console_artifact_paths) if native_console_artifact_paths else []) + args,
args = ["--server"] + select(native_server_artifact_paths) + ((["--console"] + select(native_console_artifact_paths)) if native_console_artifact_paths else []) + args,
**kwargs
)

def typedb_kt_test(name, server_mac_artifact, server_linux_artifact, server_windows_artifact,
console_mac_artifact = None, console_linux_artifact = None, console_windows_artifact = None,
native_libraries_deps = [], deps = [], data = [], args = [], **kwargs):

native_server_artifact_paths, native_server_artifact_labels = native_artifact_paths_and_labels(
server_mac_artifact, server_linux_artifact, server_windows_artifact
)

native_console_artifact_paths, native_console_artifact_labels = native_console_artifact_paths_and_labels(
console_mac_artifact, console_linux_artifact, console_windows_artifact
)

native_deps = add_native_libraries_deps(native_libraries_deps)

kt_jvm_test(
name = name,
deps = depset(deps + ["@vaticle_typedb_common//test:typedb-runner"]).to_list() + native_deps,
data = data + select(native_server_artifact_labels) + (select(native_console_artifact_labels) if native_console_artifact_labels else []),
args = ["--server"] + select(native_server_artifact_paths) + ((["--console"] + select(native_console_artifact_paths)) if native_console_artifact_paths else []) + args,
**kwargs
)

def add_native_libraries_deps(native_libraries_deps):
native_deps = []
for dep in native_libraries_deps:
native_deps = native_deps + native_dep_for_host_platform(dep)
return native_deps


def native_console_artifact_paths_and_labels(console_mac_artifact, console_linux_artifact, console_windows_artifact):
native_console_artifact_paths, native_console_artifact_labels = [], []
if console_mac_artifact and console_linux_artifact and console_windows_artifact:
native_console_artifact_paths, native_console_artifact_labels = native_artifact_paths_and_labels(
console_mac_artifact, console_linux_artifact, console_windows_artifact
)
return native_console_artifact_paths, native_console_artifact_labels

def native_artifact_paths_and_labels(mac_artifact, linux_artifact, windows_artifact):
native_artifacts = {
"@vaticle_dependencies//util/platform:is_mac": mac_artifact,
Expand Down