-
Notifications
You must be signed in to change notification settings - Fork 109
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
[763] Add missing exclusions from the CDI TCK's tck-tests.xml to the … #1195
Conversation
I've noticed I filed the issue under the wrong project, platform instead of platform-tck. I'll remedy that now and update this PR. |
@manovotn @Ladicek what do you think of the change? CC @starksm64 |
… Jakarta EE Core Profiles cdi-lite-tck-suite.xml. Signed-off-by: James R. Perkins <jperkins@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manovotn @Ladicek what do you think of the change?
CC @starksm64
Frankly, I think it's unfortunate that this file is in a completely different repository as it will be hard to keep in sync - the TCK challenges can modify the original file fairly easily so this situation is likely to repeat.
Would it be possible to instead use the original file? Genuine question as I know that it can (and currently does) contain exclusions for tests that aren't part of the core, but I am not sure if that would be an issue for testng?
As for the content of the PR, that looks good.
I think the one issue with using the original file is are the test group definitions. The <groups>
<run>
<exclude name="cdi-full"/>
<exclude name="se"/>
</run>
</groups> I'm not sure how you can add exclude groups like this. Another option, for the TCK itself, would be to use the |
Historically the exclusions list could be shipped without an update to the TCK binary, so excluding at the test level was not always viable. The current TCK rules require a full respin of the TCK binary, so maybe excluding at the source level would be better. In terms of exclusion groups, that is the syntax described in the TestNG docs: |
Ah true, didn't realize that.
The XML has the notable upside of being one place to track all exclusions. TCK actually had two source exclusions for last few years that nobody really knew about (jakartaee/cdi-tck#482). Maybe it would be enough to create some how-to-release doc for TCK that also mentions checking the exclusion file here? |
…Jakarta EE Core Profiles cdi-lite-tck-suite.xml.
Fixes Issue
Resolves #1196
Describe the change
These exclusions are in the CDI TCK, however they were not migrated to the CDI Lite TCK exclusions.
Additional context
This fix will allow the Jakarta EE Core Profile to pass, assuming the container passes :), with Java 21.
CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @shighbar @gthoman @brideck @OndroMih @dmatej
@starksm64 @scottmarlow
CC @manovotn @Ladicek