Refactor runner, adding support for kotlin #129

jamesreprise · 2022-08-18T13:12:13Z

What is the goal of this PR?

We extend TypeDB Runner to include support for tests run in Kotlin and refactor the way Runner finds passed archives for extraction and execution.

What are the changes implemented in this PR?

Previously, we passed core, cluster and console archive paths to the program and delineated them by their index. Now we pass them using command line argument flags leveraging picocli as we do across other repositories for similar purposes. For example, where would previously invoke

java <BazelTestRunner> <coreRunnerArchive> <consoleRunnerArchive>

we now invoke

java <BazelTestRunner> --server <coreRunnerArchive> --console <consoleRunnerArchive>

Under the previous implementation, it wasn't possible to run the console runner alone.

Additionally, we have added a rule typedb_kt_test which we make use of in Studio.

grabl · 2022-08-18T13:12:16Z

jamesreprise · 2022-08-18T13:13:24Z

test/Util.java

@@ -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) {


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

jamesreprise · 2022-08-18T13:15:14Z

test/Util.java

+
+    @CommandLine.Command(name = "java")
+    private static class CLIOptions {
+        @CommandLine.Parameters String mainClass;


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.

jamesreprise · 2022-08-18T13:17:20Z

test/cluster/TypeDBClusterServerRunner.java

@@ -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";


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.

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?

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

jamesreprise · 2022-08-18T13:38:10Z

test/Util.java

+            CommandLine commandLine = new CommandLine(new CLIOptions());
+            try {
+                CommandLine.ParseResult result = commandLine.parseArgs(args);
+                return Optional.of(result.asCommandLineList().get(0).getCommand());


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

alexjpwalker · 2022-08-18T14:31:01Z

test/BUILD

@@ -23,6 +23,7 @@ java_library(
    deps = [
        "//:common",
        "@maven//:commons_io_commons_io",
+        "@maven//:info_picocli_picocli",


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.

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

Let's do it now as well.

alexjpwalker · 2022-08-18T14:32:39Z

test/Util.java

        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);


Optional options, lovely. (maybe)

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...

alexjpwalker · 2022-08-18T14:47:03Z

test/rules.bzl

+           server_mac_artifact, server_linux_artifact, server_windows_artifact
+       )
+
+    native_console_artifact_paths, native_console_artifact_labels = native_console_artifact_paths_and_labels(


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.

This looks much nicer. Done.

alexjpwalker · 2022-08-18T14:56:37Z

test/rules.bzl

-    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)


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)

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.

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.

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

I've raised an issue for this:

Rename native_libraries_deps flag in typedb_java_test #130

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

alexjpwalker · 2022-08-18T15:13:58Z

test/cluster/TypeDBClusterServerRunner.java

@@ -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";


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?

alexjpwalker · 2022-08-18T15:14:53Z

test/BUILD

@@ -23,6 +23,7 @@ java_library(
    deps = [


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.

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?

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

jamesreprise · 2022-08-19T08:25:36Z

test/BUILD

@@ -23,6 +23,7 @@ java_library(
    deps = [
        "//:common",
        "@maven//:commons_io_commons_io",
+        "@maven//:info_picocli_picocli",


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.

jamesreprise added 4 commits August 18, 2022 12:20

feat: add kotlin runner test support

9d85c48

empty commit to nudge ci

bda2a6c

fix: move CLIOptions inside Util

c797c5b

fix: naming of classpath_resources was erroneously changed

bc649ac

jamesreprise added priority: blocker type: refactor labels Aug 18, 2022

jamesreprise requested a review from alexjpwalker August 18, 2022 13:12

jamesreprise self-assigned this Aug 18, 2022

jamesreprise requested review from haikalpribadi and flyingsilverfin as code owners August 18, 2022 13:12

grabl assigned flyingsilverfin, haikalpribadi and alexjpwalker Aug 18, 2022

jamesreprise commented Aug 18, 2022

View reviewed changes

fix: refactor runner bazel rules

5aa788b

jamesreprise commented Aug 18, 2022

View reviewed changes

alexjpwalker suggested changes Aug 18, 2022

View reviewed changes

jamesreprise added 2 commits August 18, 2022 17:11

fix: cleaned up names, dissolved FLAG argument into implicit functions

1449a1b

fix: native_dependencies typo

499a4b6

jamesreprise commented Aug 19, 2022

View reviewed changes

jamesreprise mentioned this pull request Aug 19, 2022

Rename native_libraries_deps flag in typedb_java_test #130

Open

alexjpwalker approved these changes Aug 19, 2022

View reviewed changes

flyingsilverfin approved these changes Aug 19, 2022

View reviewed changes

jamesreprise merged commit 3ac0c10 into typedb:master Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor runner, adding support for kotlin #129

Refactor runner, adding support for kotlin #129

jamesreprise commented Aug 18, 2022

grabl commented Aug 18, 2022 •

edited by alexjpwalker

Loading

jamesreprise Aug 18, 2022

jamesreprise Aug 18, 2022

jamesreprise Aug 18, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 18, 2022

jamesreprise Aug 18, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 18, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 19, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 18, 2022 •

edited

Loading

alexjpwalker Aug 18, 2022

jamesreprise Aug 18, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 18, 2022 •

edited

Loading

alexjpwalker Aug 18, 2022

jamesreprise Aug 19, 2022

alexjpwalker Aug 19, 2022

alexjpwalker Aug 18, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 18, 2022

alexjpwalker Aug 18, 2022

jamesreprise Aug 19, 2022

Refactor runner, adding support for kotlin #129

Refactor runner, adding support for kotlin #129

Conversation

jamesreprise commented Aug 18, 2022

What is the goal of this PR?

What are the changes implemented in this PR?

grabl commented Aug 18, 2022 • edited by alexjpwalker Loading

PR Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesreprise Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesreprise Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grabl commented Aug 18, 2022 •

edited by alexjpwalker

Loading

jamesreprise Aug 18, 2022 •

edited

Loading

jamesreprise Aug 18, 2022 •

edited

Loading