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

Add integration test against Janus #19

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Conversation

divergentdave
Copy link
Collaborator

This adds an integration test against Janus, making use of the Java testcontainers library to manage the lifetime of our Janus interoperation test containers. This closes #13.

@divergentdave divergentdave requested a review from a team as a code owner November 13, 2023 16:09
Copy link
Contributor

@inahga inahga left a comment

Choose a reason for hiding this comment

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

Nicely done!


private static TaskId randomTaskId() {
byte[] bytes = new byte[32];
new Random().nextBytes(bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly nit, can leave it if you don't think it's pertinent: java.util.Random is not a CSPRNG.

For practical usage in these tests it probably doesn't matter, but I would be worried about someone reading these tests for reference on how to use the library, and copying over usage of java.util.Random.

);
/** @noinspection SpellCheckingInspection */
private static final DockerImageName JANUS_INTEROP_COLLECTOR = DockerImageName.parse(
"us-west2-docker.pkg.dev/divviup-artifacts-public/janus/janus_interop_collector@sha256:982110bc29842639355830339b95fac77432cbbcc28df0cd07daf91551570602"
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: why are these SHAs rather than tagged versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did it for consistency with divviup-ts, but I now see that we have both the tag and SHA over there. I'll update this to match.

@divergentdave
Copy link
Collaborator Author

The Java version of testcontainers has some interesting features that might be worth copying in Janus. First off, they start up a daemon called Ryuk in a container that does last-resort container cleanup if the test process disconnects and doesn't clean up after itself for whatever reason. Second, they have a feature that allows exposing a TCP server on a host port within a container network. Under the hood, this works by running sshd in a container, attached to the correct Docker network, and running ssh on the host with reverse tunnel options set. This could allow for new combinations of in-process an containerized aggregators in our integration tests.

@divergentdave divergentdave merged commit f0fceb0 into main Nov 13, 2023
2 checks passed
@divergentdave divergentdave deleted the david/testcontainers branch November 13, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration test against Janus
3 participants