eclipse / microprofile-telemetry

microprofile telemetry
Apache License 2.0
19 stars 18 forks source link

Move from Junit to TestNG #61

Closed yasmin-aumeeruddy closed 1 year ago

yasmin-aumeeruddy commented 1 year ago

We have:

Azquelt commented 1 year ago

The problem with RestAssured was not so much trying to reduce the number of dependencies in the TCK, but just getting it correctly included in the test application. It does have a lot of dependencies, some of which are optional depending on your environment. E.g. it lists the javax and jakarta versions of the JAX-B API as optional dependencies but I think it needs at least one of them since I got class not found exceptions for JAX-B classes.

I tried using the shrinkwrap maven resolver to pull it in with its dependencies, but I was still getting CNFE for JAX-B classes. I assume I just needed to add some of the optional dependencies, but I wasn't confident in getting it right for all environments where the TCK needs to run so in the end it was just easier to replace it with HttpUrlConnection since we're not using and of the complex facilities of RestAssured, we just need something that will make an HTTP request from the server without triggering MP Telemetry to create a new span.

I'd be happy to go back to using RestAssured if we can get it packaged in the deployment correctly, or if we change the test structure so that we're making requests from the client-side, rather than from within the server.

Edit: forgot links

brunobat commented 1 year ago

@Azquelt if it works now, let it be like you did it. Thanks for the explanation.

JanWesterkamp-iJUG commented 1 year ago

@yasmin-aumeeruddy, @Azquelt and @Emily-Jiang, I recognised you converted the test from JUnit 5 to TestNG now? Can you explain why? The only way to let the tests pass in OpenLiberty?

Now I have to redo my changes for OpenTelemetry Java Instrumentation 1.19.0: https://github.com/eclipse/microprofile-telemetry/pull/60 (original) https://github.com/eclipse/microprofile-telemetry/pull/64 (additional requested changes)

I will try to do the rebase tomorrow.

yasmin-aumeeruddy commented 1 year ago

@JanWesterkamp-iJUG This was a change that was decided in the meeting on September 12th. @brunobat pointed out that all of the other TCK tests are implemented with TestNG and he had come across problems with Junit

brunobat commented 1 year ago

Yes, this was explained and agreed on a meeting

JanWesterkamp-iJUG commented 1 year ago

@yasmin-aumeeruddy and @brunobat: I missed that meeting unfortunately.

And it was questioned on the meeting from the 2022-10-03. But what was the real reason for it? I tried to get some details here too: https://github.com/eclipse/microprofile-telemetry/issues/53

Regarding the exclusive use of TestNG: I.e. MP Metrics and MP Reactive Messaging are using JUnit 4, MP GraphQL (tck/client) and Jakarta REST are using JUnit 5. This is mainly because the MP Parent was not maintained very well regarding support of JUnit 5 at the moment only.

Following the comments here it looked like the main blocker was RestAssured with OpenLiberty - not JUnit 5 (or TestNG). Or was there something else?

brunobat commented 1 year ago

@JanWesterkamp-iJUG this is a practical issue. If you can produce a build of OpenLiberty running the TCK with JUnit, please create the PR. Otherwise we are going with TestNG because it's the solution that currently works.