Skip to content

Conversation

@dhoard
Copy link

@dhoard dhoard commented Oct 10, 2024

Summary

Added Network.newNetwork(boolean initialize) method to force initialization on creation.

Purpose

When writing integration tests, I want to confirm that the network has been created before proceeding.

The current API requires two method calls:

Network network = Network.newNetwork();
network.getId();

The new method would allow a single method call:

Network network = Network.newNetwork(true);

Real-world example

Prometheus JMX Exporter integration tests

Using the existing netNetwork method:

    @Verifyica.Prepare
    public static void prepare(ClassContext classContext) {
        if (classContext.testArgumentParallelism() == 1) {
            // Create the network at the test class scope
            // Get the id to force the network creation
            Network network = Network.newNetwork();
            network.getId();

            classContext.map().put(NETWORK, network);
        }
    }

Reference:

https://github.com/prometheus/jmx_exporter/blob/89949413148adcfbc8ded41a2d931c8ef608ab2d/integration_test_suite/integration_tests/src/test/java/io/prometheus/jmx/test/common/AbstractExporterTest.java#L65-L75


Using the new newNetwork(boolean initialize) method:

    @Verifyica.Prepare
    public static void prepare(ClassContext classContext) {
        if (classContext.testArgumentParallelism() == 1) {
            // Create the network at the test class scope
            classContext.map().put(NETWORK, Network.newNetwork(true);
        }
    }

@dhoard dhoard requested a review from a team October 10, 2024 13:25
@kiview
Copy link
Member

kiview commented Oct 16, 2024

Thanks @dhoard, I don't understand why you can't share the Network object without initialization, given that getId() is synchronized and should not lead to any synchronization issues in parallel usage.

So what is the strong need for this?

When writing integration tests, I want to confirm that the network has been created before proceeding.

@dhoard
Copy link
Author

dhoard commented Oct 16, 2024

@kiview The goal is to be able the verify the Network is created to prevent unnecessary work setting up multiple test containers (map/copy files, etc.) only for the network to fail.

AnewNetwork method that doesn’t really allocate the network seems odd to me. Since changing the default behavior could have unknown consequences, I proposed the method with a variable.

—-

Real world usage is testing the Prometheus JMX Exporter.

https://github.com/prometheus/jmx_exporter/blob/main/integration_test_suite/integration_tests/src/test/java/io/prometheus/jmx/test/common/AbstractExporterTest.java

https://github.com/prometheus/jmx_exporter/blob/main/integration_test_suite/integration_tests/src/test/java/io/prometheus/jmx/test/MinimalTest.java

@eddumelendez
Copy link
Member

Thanks for your contribution, @dhoard. This is the expected behavior and the workaround is valid. The lazy network initialization is how it is supposed to work.

I think understanding the reason about the network not being created should be revisited instead. i.e. networks not being clean after test executions could be one reason.

@dhoard dhoard deleted the new-network-initialize-method branch November 26, 2024 20:23
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.

3 participants