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

4.x: Helidon Arquillian module should only depend on MP core #7440

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Aug 24, 2023

Description

Issue #6005

Documentation

None

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 24, 2023
@jbescos jbescos force-pushed the issue6005 branch 6 times, most recently from 4214101 to dc26013 Compare August 24, 2023 14:11
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@@ -70,5 +64,9 @@
<groupId>org.testng</groupId>
<artifactId>testng</artifactId>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Why does it depend on MP metrics?

Copy link
Member Author

@jbescos jbescos Aug 25, 2023

Choose a reason for hiding this comment

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

It fails otherwise with some injection error I think because of HelidonDeployableContainer#cleanupBaseMetrics, Right now arquillian-helidon requires it.

Update: see the error in the next comment

<artifactId>helidon-microprofile-metrics</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

This is part of helidon MP core, no need to add it

Copy link
Member Author

@jbescos jbescos Aug 25, 2023

Choose a reason for hiding this comment

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

There are test failures:

[ERROR] Tests run: 3, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.161 s <<< FAILURE! - in org.jboss.cdi.tck.tests.context.dependent.transientreference.DependentTransientReferenceDestroyedTest
[ERROR] org.jboss.cdi.tck.tests.context.dependent.transientreference.DependentTransientReferenceDestroyedTest.arquillianAfterClass  Time elapsed: 0.01 s  <<< FAILURE!
org.jboss.weld.exceptions.UnsatisfiedResolutionException: WELD-001334: Unsatisfied dependencies for type MetricRegistry with qualifiers @RegistryType 
	at org.jboss.weld.bean.builtin.InstanceImpl.checkBeanResolved(InstanceImpl.java:251)
	at org.jboss.weld.bean.builtin.InstanceImpl.get(InstanceImpl.java:113)
	at io.helidon.microprofile.arquillian.HelidonDeployableContainer.cleanupBaseMetrics(HelidonDeployableContainer.java:583)

mvn dependency:tree does not show helidon-microprofile-metrics

@@ -36,6 +36,11 @@
<artifactId>helidon-microprofile-config</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here for config TCK? AFAIK metrics are not used from config

Copy link
Member Author

@jbescos jbescos Aug 25, 2023

Choose a reason for hiding this comment

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

As long as you have this dependency, you need metrics:

        <dependency>
            <groupId>io.helidon.microprofile.tests</groupId>
            <artifactId>helidon-arquillian</artifactId>
        </dependency>

We could add it directly in Helidon-Arquillian but I wanted to remove any microprofile dependency from there. I hace the feeling that eventually we will remove it or we will redesign it from helidon-arquillian.

@@ -30,6 +30,11 @@
<name>Helidon Microprofile Tests TCK Core Profile</name>

<dependencies>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

why do you depend on the whole thing here? It should be sufficient to depend on the core, as core profile should be already supported by core.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, for some reason I thought this was testing a small parts of different microprofile things.

<artifactId>helidon-microprofile-health</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Metrics seem to be added to all poms, including the arqulilian. Please explain

Copy link
Member Author

Choose a reason for hiding this comment

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

See the first comments. It is required if you use Helidon Arquillian.

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos
Copy link
Member Author

jbescos commented Aug 25, 2023

I have updated helidon-arquillian to not require metrics in every TCK

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos jbescos changed the title Helidon Arquillian module should only depend on MP core 4.x: Helidon Arquillian module should only depend on MP core Aug 25, 2023
@jbescos jbescos merged commit f24c47f into helidon-io:main Aug 25, 2023
12 checks passed
dalexandrov pushed a commit to dalexandrov/helidon that referenced this pull request Aug 26, 2023
…-io#7440)

Helidon Arquillian module should only depend on MP core

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants