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

ci: moves ActiveMQ to docker based tests #220

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Conversation

codefromthecrypt
Copy link
Member

this removes a junit 4 dependency as well

codefromthecrypt pushed a commit to openzipkin/zipkin that referenced this pull request Dec 13, 2023
This replaces the in-VM approach for testing activemq which has been
flakey and also results in a JUnit 4 rule dependency.

Note: This uses the JDK image, not the JRE, because ActiveMQ relies on
JMX even when disabled via configuration. This is a better choice than
making our production JRE image include it.

Once this is deployed I can make zipkin-reporter use it per below:
 openzipkin/zipkin-reporter-java#220

I can also change our collector tests here to use it instead of the
JUnit 4 rule.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
return "failover:tcp://" + container.getHost() + ":" + container.getMappedPort(ACTIVEMQ_PORT);
}

// mostly waiting for https://github.com/testcontainers/testcontainers-java/issues/3537
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may be copy-pasted and no need to change anything now, but just for reference, @TestContainers and @Container automatically skip when docker isn't available. It would look something like this, but don't think it's a priority to change anything

Rename ActiveMQExtension -> ActiveMQContainer

public static class ActiveMQContainer extends ... {
  public ActiveMQContainer() {
    ...
  }

  public String newSenderBuilder(String queue) {
    ...
  }
}

@Testcontainers
public class ITActiveMQSender {
  @Container public static final activemq = new ActiveMQContainer();

...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

does this also allow property override? in the main zipkin build we intentionally skip docker on one pass so we can parallelize them...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if we could do what you said and still have a skip property (even if we make it), sounds much better and I can help sweep around and apply the pattern everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try a separate PR once this is in to do all of them. thanks for the tip @anuraaga!

codefromthecrypt added a commit to openzipkin/zipkin that referenced this pull request Dec 13, 2023
…3636)

This replaces the in-VM approach for testing activemq which has been
flakey and also results in a JUnit 4 rule dependency.

Note: This uses the JDK image, not the JRE, because ActiveMQ relies on
JMX even when disabled via configuration. This is a better choice than
making our production JRE image include it.

Once this is deployed I can make zipkin-reporter use it per below:
 openzipkin/zipkin-reporter-java#220

I can also change our collector tests here to use it instead of the
JUnit 4 rule.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt marked this pull request as ready for review December 13, 2023 13:23
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt merged commit 4926501 into master Dec 14, 2023
2 checks passed
@codefromthecrypt codefromthecrypt deleted the activemq-docker branch December 14, 2023 00:58
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