CDITests.testCDIInjectContexts

[chengfang]

The relevant specification version and section number(s): 2.1

The coordinates of the challenged test(s): com.ibm.jbatch.tck.tests.jslxml.CDITests.testCDIInjectContexts

The exact TCK version: 2.1

The implementation being tested, including name and company: JBeret, by Red Hat, Inc.

A full description of why the test is invalid and what the correct behavior is believed to be:
Any supporting material; debug logs, test output, test logs, run scripts, etc.:

I noticed 2 issues with TCK test CDITests.testCDIInjectContexts:

1, this test uses a batchlet DependentScopedBatchletContexts which injects JobContext and StepContext in various ways (field, constructor, and method injections). But all fields are already annotated with @Inject, which means they are already field-injected. So even if one implementation does not support method or constructor injection, the target field already contains the expected value. So we should tighten this test by removing the @Inject from those fields receiving constructor or method injections.

2, step exit status from process() method return value, or StepContext.setExitStatus(...)?

A batchlet step can get its exit status from either process() method return value, or StepContext.setExitStatus(), but which takes precedence? The test assumes setExitStatus(...) wins and ignore the return value. I couldn't find a clear language in the spec.

One may argue that setExitStatus() call is more explicit so it should win. But return value from process() is equally strong in this regard, since this is exactly the purpose of this return value, and no other use of this return value exists.

I suggest we change the test such that the batchlet process method returns the same value as the value in setExitStatus(), and the test always receives the consistent exit status.

[ERROR] Failures:
[ERROR]   CDITests.testCDIInjectContexts:116 Test fails - unexpected step exit status ==> expected: <13:13:13:> but was: <OK>
[ERROR]   CDITests.testCDIInjectContexts:116 Test fails - unexpected step exit status ==> expected: <14:14:14:> but was: <OK>
[ERROR]   CDITests.testCDIInjectContexts:116 Test fails - unexpected step exit status ==> expected: <15:15:15:> but was: <OK>

[scottkurz]

Noting my response from the ML: https://www.eclipse.org/lists/jakartabatch-dev/msg00297.html, I agree with the challenge.

It's a shame we didn't catch the misinterpretation of batchlet return value as exit status earlier but we should at least change the TCK test as Cheng suggested.

[scottmarlow]

Will a new TCK be released to address this?

[chengfang]

@scottkurz This issue causes 6 test failures in total (3 in ejb vehicle, 3 in web vehicle). I assume we'd want to patch it in a new maintenance release, since the other option (excluding them) seems loss of important coverage. Do you have ETA when such patch will be available? Thanks!

[scottkurz]

I didn't want to give an ETA until I was actually working on this. I am now.

In reading the TCK process it seems there's no option to accept a challenge by fixing or modifying a test. The only option (besides rejecting) is to declare an exclusion.

I can certainly see the logic in such an interpretation, since otherwise already certified implementations could potentially have to re-run against the updated test(s).

@scottmarlow is that your take on this as well?

Anyway, let me give some thought on how to translate the "exclude list" idea into something concrete.

FWIW, I hadn't realized how prescriptive the TCK process was on the subject of the exclude list:

Adding excluded tests

Excluded tests should be added back in for every major Jakarta EE release by emptying the test exclude list for every Specification developed in the respective major EE release.

One might say that by having @Disabled-annotated tests in the TCK master source, we're kind of in violation of the spirit of that. On the other hand, we could say the "exclude list" is just a doc thing.. and it's empty :) Not sure how much discussion has been had on this and how strongly people feel.

In any case, I'll put together something for review in this PR and add you, Scott, and @chengfang as reviewers.

Thanks.

[scottmarlow]

In reading the TCK process it seems there's no option to accept a challenge by fixing or modifying a test. The only option (besides rejecting) is to declare an exclusion.

I can certainly see the logic in such an interpretation, since otherwise already certified implementations could potentially have to re-run against the updated test(s).

@scottmarlow is that your take on this as well?

Please look at the last sentence that I am copying from the TCK Process document:

Accepted Challenges

A consensus that a test produces invalid results will result in the exclusion of that test from certification requirements, and an immediate update and release of an official distribution of the TCK including the new exclude list. The associated challenge issue MUST be closed with an accepted label to indicate it has been resolved.

The specification project may approve (user) workarounds for an accepted TCK challenge (as alternative to excluding TCK tests).

The last sentence is general enough that you could manage creating TCK updates that:

1. As you said, exclude test(s) for which challenges have been accepted. This is the easiest approach that will not impact Batch implementations that have already passed the Batch TCK. You can make your test changes in the next Batch SPEC release.
2. If you have a TCK change that you think is safe for Batch implementations that have already passed the TCK, you could update the test to address the challenge, however if the test change breaks an existing Batch implementation and that implementation challenges the (updated) test, your only option would be to exclude the test as it really wasn't a safe change after all.
3. Instead of updating an existing test, add a new test and ensure that Batch implementations could run either the challenged test or the newly added test instead.

If you have feedback on ^ please share feedback your thoughts on https://www.eclipse.org/lists/jakarta.ee-spec mailing list as #2 + #3 is something that we just talked about at the Jakarta Specification call this week. The Platform TCK mailing list could also be used for feedback on ^ as an alternative to discussing your feedback on this issue. Hope that helps!

[scottkurz]

2. If you have a TCK change that you think is safe for Batch implementations that have already passed the TCK, you could update the test to address the challenge, however if the test change breaks an existing Batch implementation and that implementation challenges the (updated) test, your only option would be to exclude the test as it really wasn't a safe change after all.

Thank you @scottmarlow for adding the detailed comments.

Yes, I absolutely do feel like this change in safe and so will treat this as case 2. in your comment, and work to fix the test in a 2.1 TCK update (v2.1.1).

The rationale here:

1. The logic of the test change is more inclusive: to the extent the issues are understood . (This is true with respect to the step exit status at least, though not true with respect to the removal of the @Inject annotations... but it'd be hard to see a failure there as anything other than a bug.)
2. There is only one implementation currently certified: jbatch (There are no EE 10 platform certifications currently listed at jakarta.ee)
3. I tested (locally) the changes against each of: jbatch, Open Liberty and GlassFish 7.0.0-M5 which all pass.

Still, there's always room for error and the introduction of some risk. I may have done the manual testing incorrectly or made an incorrect assumption in this chain of logic. But... we did get PR reviews. And... there'd be similarly some amount of risk for updating an automated exclude mechanism.

---

I'll be happy to give feedback on one of the suggested forums when this is done. Thanks again.