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

Added newNetwork method #9371

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

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 as a code owner 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

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.

2 participants