TCK Challenge: Server Async Tests

[jasondlee]

The WildFly team would like to challenge the tests in JaxRsServerAsyncTest. These tests and the functionality they exercise require two Jakarta EE specs, Servlet and Concurrency, that are not offered/require by the MP Platform spec, so it is inappropriate for the MP Telemetry spec to require them. This test class, then, should be removed.

[Emily-Jiang]

@jasondlee these tests are optional. You don't need to pass them. You can skip them. See this instruction.

[jasondlee]

@Emily-Jiang Right, and I've done that, but the TCK does state Although support for JAX-RS server async programming models is not optional...

Regardless, the test themselves probably should not be in the TCK even if they're ignorable for the reasons above.

[Emily-Jiang]

@jasondlee The tests have the group @Test(groups = "optional-jaxrs-tests") here. This pattern was used by both Jakarta EE and MicroProfile.. We could investigate to create a separate jar for optional tests.

[jasondlee]

Perhaps I'm being overly pedantic, but as I understand things, the TCK is a set of tests one can use to verify that an implementation is compliant with a specification. The existence of a test in the TCK, then, is explicitly stating that the functionality under test is detailed in the specification. As best as I can tell, this functionality is mentioned in neither place, so the tests, optional or not, are inappropriate. They also require Jakarta EE specs not listed in the platform spec, as previously noted, making them inappropriate for another reason. It seems to me that the correct thing to do is not to move them to an optional jar, but to remove them altogether (or modify the relevant specs, but that would be a new spec version and wouldn't apply here).

[Emily-Jiang]

By the way, this challenge is invalid as you don't need to pass these tests to be certified. We will investigate whether to automatically skip these tests if a Servlet class cannot be loaded for an instance.

[jamezp]

@Emily-Jiang Is there a document that states what makes this an invalid challenge? Ignored or not, the explanation above sure seems like this is a valid challenge.

[Emily-Jiang]

@Emily-Jiang Is there a document that states what makes this an invalid challenge? Ignored or not, the explanation above sure seems like this is a valid challenge.

Here is the TCK process.
Invalid challenges:

Challenges to tests that are already excluded for any reason.

These tests are excluded from certification though. I think you challenged not to include any optional tests, which is different topic. Please bring up to the MP google mailing for a wider discussion as it affects all of MP specs.

[jasondlee]

I think you challenged not to include any optional tests, which is different topic.

My actually challenge was that the test themselves shouldn't be there in the first place. From the linked document

Claims that a test asserts requirements over and above that of the specification.

Based on that and the initial comment:

These tests and the functionality they exercise require two Jakarta EE specs, Servlet and Concurrency, that are not offered/require by the MP Platform spec, so it is inappropriate for the MP Telemetry spec to require them.

I think this is a valid challenge.

[jamezp]

The second point in https://github.com/eclipse/microprofile-telemetry/blob/main/tracing/tck/README.adoc#declaring-the-tests-to-run makes these tests almost appear not optional in an Jakarta EE container though.

It seems really odd to me to keep a test which intermittently fails because it can be ignored in non-Jakarta EE environments.

Excluding the test is fine, but it does seem the tests should be addressed and not simply rejected. I guess the question would be are these tests truly optional?

[Emily-Jiang]

I think you challenged not to include any optional tests, which is different topic.

My actually challenge was that the test themselves shouldn't be there in the first place. From the linked document

Claims that a test asserts requirements over and above that of the specification.

Based on that and the initial comment:

These tests and the functionality they exercise require two Jakarta EE specs, Servlet and Concurrency, that are not offered/require by the MP Platform spec, so it is inappropriate for the MP Telemetry spec to require them.

I think this is a valid challenge.

@jasondlee my point is that these tests are not required.

[Emily-Jiang]

The second point in https://github.com/eclipse/microprofile-telemetry/blob/main/tracing/tck/README.adoc#declaring-the-tests-to-run makes these tests almost appear not optional in an Jakarta EE container though.

It seems really odd to me to keep a test which intermittently fails because it can be ignored in non-Jakarta EE environments.

Excluding the test is fine, but it does seem the tests should be addressed and not simply rejected. I guess the question would be are these tests truly optional?

These tests are truly optional.

[xstefank]

IMHO, an optional test is not by default excluded test, so it should be possible to challenge optional tests.

[Emily-Jiang]

IMHO, an optional test is not by default excluded test, so it should be possible to challenge optional tests.

You can raise an issue to report an incorrect test. However the test itself is correct. For challenges, it is pretty much for certification purpose from my understanding. I will seek clarification from Eclipse Specification process to see whether a challenge is the right category.

[JanWesterkamp-iJUG]

@jasondlee this in a valid challenge and could have been prevented, as this is linked to my Release Review finding #132 and caused me to vote against the Release.

As described there, the current setup is a severe architecural constraint violation and deactivating the tests in enough, also dependencies need to be removed from the TCK!
The requirement of editing the TCK manually to be able to pass the tests as a valid implementation are a invalid shortcut - this must be changed.

[Azquelt]

These tests and the functionality they exercise require two Jakarta EE specs, Servlet and Concurrency

I agree it requires Concurrency, I didn't think it required Servlet but I may have missed something there. Can you point me to something in the test that requires servlet?

the TCK is a set of tests one can use to verify that an implementation is compliant with a specification

I think this is the key point. Like any test suite, a TCK will never be able to cover 100% of cases. Just because you pass the TCK, does not mean that you have implemented the spec completely and correctly.

However, we strive to ensure that coverage is as good as it can be which helps both users and implementors:

• users because it means that whatever implementation they pick up is less likely to have bugs or missing features
• implementers because it helps to catch anything they may have missed before users find it

Adding these tests in was a compromise. If you do happen to implement Jakarta EE Concurrency, then you can use these tests to verify that your implementation is not broken in the specific way this test runs. If you don't implement Jakarta EE Concurrency then you should disable these tests and ensure that this area is covered in your own internal testing.

They're enabled by default because TestNG doesn't give you an easy way to disable test groups by default. We could probably work around that by having implementers set a system property or something to enable the tests instead, but that doesn't seem to be the issue here since you're asking for the tests to be removed altogether.

I do think these particular tests are important, it's something that's easy to miss, tricky to get right and will annoy users - no-one wants to deal with their tracing breaking because they re-wrote part of their code to use JAX-RS async methods. We spotted it late in development in our Telemetry 1.0 implementation and wrote our own tests for it but we would have caught it earlier if it had been covered by the TCK. Adding these optional tests was done out of a desire to help implementers who do support Jakarta EE Concurrency to spot these problems earlier.

MicroProfile is a community effort so if the community decides we don't want any optional tests then they should be removed. However, I think this area is important enough that we would have to find another way of testing it. My initial thoughts are that we would ask the test runner to implement an interface that provided us an executor with which to run async tasks. This is similar to the "porting package" that must be provided to run the Jakarta EE CDI TCK.

This would be very simple for implementers to provide, though it would be an additional step required to run the TCK. Implementations that support Jakarta EE Concurrency could return a ManagedExecutorService, implementations that allow use of unmanaged threads could return the common fork-join pool or a manually constructed thread pool, implementations that have another way of running things asynchronously could do that.

I'd also be open to any other suggestions for how to test that JAX-RS async resource methods are correctly traced.

[brunobat]

I've checked in Quarkus and it should be totally fine to rely on a MP Context Propagation's ManagedExecutor to run the async TCK tests. @Azquelt @Emily-Jiang. If this simplifies everyone's life, I'm ok with it.

[brunobat]

I also think we shouldn't rely on Jakarta Concurrency or Servlet. These test are important and we should do an effort to modify them in a more portable way.

[Azquelt]

Raised #140 to rewrite this test to allow the implementer to provide whatever async mechanism is most appropriate for their runtime when they run the TCK.

[JanWesterkamp-iJUG]

As mentioned in the last meeting, here are the slides with some patterns for the solution:

Current situation:

Split TCK into optional moduls solution:

Split TCK into separate MP Telemetry Integration spec solution:

[Emily-Jiang]

fixed via #183