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

[763] Add missing exclusions from the CDI TCK's tck-tests.xml to the … #1195

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

jamezp
Copy link
Contributor

@jamezp jamezp commented Oct 13, 2023

…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

@jamezp
Copy link
Contributor Author

jamezp commented Oct 13, 2023

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.

@scottmarlow
Copy link
Contributor

@manovotn @Ladicek what do you think of the change?

CC @starksm64

@jamezp jamezp changed the base branch from master to 10.0.x October 13, 2023 15:49
… Jakarta EE Core Profiles cdi-lite-tck-suite.xml.

Signed-off-by: James R. Perkins <jperkins@redhat.com>
Copy link
Contributor

@manovotn manovotn left a 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.

@jamezp
Copy link
Contributor Author

jamezp commented Oct 13, 2023

I think the one issue with using the original file is are the test group definitions. The cdi-lite-tck-suite.xml has:

<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 @Test(enabled = false). Not as ideal I know, but it would be more "global".

@starksm64
Copy link
Contributor

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:
https://testng.org/doc/documentation-main.html#exclusions

@starksm64 starksm64 merged commit ed6f644 into jakartaee:10.0.x Oct 13, 2023
@manovotn
Copy link
Contributor

I think the one issue with using the original file is are the test group definitions. The cdi-lite-tck-suite.xml has:

Ah true, didn't realize that.

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.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tck-challege] Jakarta EE Core TCK's cdi-lite-tck-suite.xml out of sync with CDI's tck-tests.xml
4 participants