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

Update TCK for web profile #260

Merged
merged 2 commits into from
Jul 30, 2022
Merged

Conversation

KyleAure
Copy link
Contributor

@KyleAure KyleAure commented Jul 28, 2022

Fixes #244

I went ahead and duplicated the test classes that currently can only run on Full profile because they package tests as EARs, and packaged them as WEB archives instead. I then disabled tests where they didn't make sense to run on Web Profile.

I also had to make some modifications due to the way that we passed EJB JNDI names between the test classes and the application running on the server. I did this by introducing an SPI that will provide the JNDI name depending on if we are running in Full Profile vs Web Profile.

Here are the results of testing this against Open Liberty:

Full Profile Results

Screen Shot 2022-07-28 at 2 35 58 PM

Web Profile Results

Screen Shot 2022-07-28 at 2 26 57 PM

Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this pull Kyle! It looks like you overachieved and were able to package more tests than just those 4 servlets with 3.0 spec coverage into WARs for web profile testing.

@scottmarlow
Copy link

Previously the concurrency/tck-dist/src/main/starter/suite-web.xml had:

        <groups>
            <run>
                <exclude name="eefull" />
            </run>
        </groups>

But now,
concurrency/tck-dist/src/main/starter/suite.xml has:

        <groups>
	      <run>
	        <exclude name="eeweb"/>
	      </run>
	    </groups>

Should the concurrency/tck-dist/src/main/asciidoc/concurrency-tck-reference-guide.adoc give guidance on running the Concurrency TCK with Web Profile versus Full Platform?

Previously, concurrency-tck-reference-guide.adoc contained:

These "suites" are both represented within the single `artifacts/suite.xml` file for the full profile and `artifacts/suite-web.xml` for the web profile, and are identified by their package names:

1. ee.jakarta.tck.concurrent.api.*
1. ee.jakarta.tck.concurrent.spec.*

*Note:* An implementation **MUST** run against all tests provided in the suite XML file for the given profile [underline]#unmodified# for an implementation of such profile to pass the TCK.

With this pull request, we now have:

These "suites" are both represented within the single `artifacts/suite.xml` file and are identified by their package names:

1. ee.jakarta.tck.concurrent.api.*
1. ee.jakarta.tck.concurrent.spec.*

*Note:* An implementation **MUST** run against all tests provided in the suite XML file [underline]#unmodified# for an implementation to pass the TCK.

Should we add guidance on switching between running against Web Profile versus Full Platform like we had before? Or do we not need that anymore?

Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite work, at least in WildFly. There are two tests, ManagedExecutorDefinitionWebTests and ManagedScheduledExecutorDefinitionWebTests, which attempt to register the same ManagedExecutorDefinition twice. It really seems they should be creating different resource names or the packages themselves cannot be added.

I will note it's not enough to just remove the package import as both the servlet and EJB context URL's are injected to the test via Arquillian.

One minor nit, the formatting is using a combo of tabs and spaces. It seems like ideally it would be consistent.

ContextServiceDefinitionServlet.class,
ContextServiceDefinitionInterface.class,
ContextServiceDefinitionWebBean.class)
.addAsWebInfResource(ManagedScheduledExecutorDefinitionWebTests.class.getPackage(), "ejb-jar.xml", "ejb-jar.xml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this, it's registering the same @ManagedExecutorDefinition.

@arjantijms
Copy link
Contributor

Running the web profile against GF results in one failure:

[ERROR] Failures: 
[ERROR]   ManagedExecutorDefinitionWebTests>Arquillian.run:138->testCopyCompletableFutureEJB:105->TestClient.runTest:70->TestClient.assertSuccessfulURLResponse:143->ArquillianTests.assertTrue:108 Output did not contain successful message: SUCCESS expected [true] but found [false]
[INFO] 
[ERROR] Tests run: 148, Failures: 1, Errors: 0, Skipped: 0

I'll have to look into this to see what is exactly going wrong.

@arjantijms
Copy link
Contributor

There's some concerns still, but let's address those in a next PR.

@KyleAure
Copy link
Contributor Author

KyleAure commented Aug 1, 2022

@scottmarlow

Should we add guidance on switching between running against Web Profile versus Full Platform like we had before? Or do we not need that anymore?

I didn't make any changes to the TCK doc besides fixing the suite.xml file to only run full tests.
In combination with #259 I think the documentation is cohesive now. Let me know if there are any other changes you would like to see.

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 uses remote EJBs
7 participants