Alternative fix for #244 #250

starksm64 · 2022-07-06T16:36:17Z

This is a simplified fix for #244 challenge that allows one to exclude EAR based tests using the eefull group, and changes jakarta.ejb.Remote interfaces into simple interfaces for local usage. It passes with GlassFish 7 snapshot build:

Full platform:
===============================================
jakarta-concurrency
Total tests run: 149, Passes: 149, Failures: 0, Skips: 0
===============================================

[INFO] Tests run: 149, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 288.889 s - in TestSuite
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 149, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] 

Web profile with eefull group excluded:
===============================================
jakarta-concurrency
Total tests run: 100, Passes: 100, Failures: 0, Skips: 0
===============================================

[INFO] Tests run: 100, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 211.601 s - in TestSuite
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 100, Failures: 0, Errors: 0, Skipped: 0

Signed-off-by: Scott M Stark starksm64@gmail.com

Signed-off-by: Scott M Stark <starksm64@gmail.com>

arjantijms · 2022-07-06T17:02:03Z

For now 100 tests for web profile seems fine. It's almost exactly 2/3. For a next release we can maybe see how many more can be made web profile friendly, as this seems to exclude a few tests too many still.

arjantijms

I would maybe actually remove the commented-out lines instead of commenting them out, but it's not a deal breaker.

Signed-off-by: Scott M Stark <starksm64@gmail.com>

starksm64 · 2022-07-06T18:00:43Z

I was just waiting on some feedback. The commented out statements have been removed.

brideck · 2022-07-06T18:26:38Z

Also not a deal breaker, but if this is the solution used, an issue should be opened against this TCK to at least rename all of the EJB interfaces in a future release. Speaking from personal experience, having interface names of BlahRemote when the interfaces aren't actually Remote anymore will be confusing in the long run.

Signed-off-by: Scott M Stark <starksm64@gmail.com>

KyleAure

I think this is the better fix of the two proposed where we can keep the behavior of the tests in full profile while also providing a subset of tests that we expect to work in web profile. In future releases, we can fill in some gaps we have in web profile testing that only exist in full profile today.

njr-11 · 2022-07-12T16:30:03Z

...a/tck/concurrent/spec/ManagedExecutorService/resourcedef/ManagedExecutorDefinitionTests.java

@@ -34,6 +34,9 @@
 import ee.jakarta.tck.concurrent.spi.context.StringContextProvider;
 import jakarta.enterprise.concurrent.spi.ThreadContextProvider;

+import static ee.jakarta.tck.concurrent.common.TestGroups.JAKARTAEE_FULL;
+
+@Test(groups = JAKARTAEE_FULL)
 public class ManagedExecutorDefinitionTests extends TestClient{


If I'm reading this correctly, it looks like all of the tests for new spec function within the following servlets have been moved to full profile:
ContextServiceDefinitionServlet.java
ManagedExecutorDefinitionServlet.java
ManagedScheduledExecutorDefinitionServlet.java
ManagedThreadFactoryDefinitionServlet.java

Are there reasons why these must be excluded from web profile? Otherwise, everything else under this pull looks great.

What determined whether or not a test was marked as requiring the full platform was whether it used an EAR as the deployment archive. The only tests that include these 4 servlets are using EARs.

Can these tests be updated to use WAR instead of EAR?

I don't expect it will be valid to convert those existing WARs into EARs without deleting other coverage that required the EAR in the first place. However, it might be possible to keep the existing EARs for full profile while also packaging those same servlets into separate WARs where the tests could be targeted to web profile. Is that sort of thing possible?

Not as a service release under the current tck process as new tests or changes to tests are not supported.

Not as a service release under the current tck process as new tests or changes to tests are not supported.

By that standard, it seems the updates already under this pull to convert Remote EJBs to Local EJBs would be disallowed even more so in a service release. Simply adding the existing test servlets unchanged into a WAR so that they can run under web profile would be far less intrusive than that because it does not change the existing tests at all and only fixes packaging which gets the test to stop failing in web profile.

Here's another approach that might fit better with the process:
Instead of marking ManagedExecutorDefinitionTests as JAKARTAEE_FULL, could individual tests within it, such as testCopyCompletableFutureEJB be marked JAKARTAEE_FULL? And then if createDeployment could check for full vs web, it could create the EAR for the former and WAR for the latter. Would that be more acceptable if it's possible?

If the concurrency tck team can propose a service release that maximizes the test coverage under the web profile, an exception as a service release could be requested. In this particular case, I don't see a problem with that since no web profile implementation has been able to pass.

@njr-11 @starksm64

Instead of marking ManagedExecutorDefinitionTests as JAKARTAEE_FULL, could individual tests within it, such as testCopyCompletableFutureEJB be marked JAKARTAEE_FULL? And then if createDeployment could check for full vs web, it could create the EAR for the former and WAR for the latter. Would that be more acceptable if it's possible?

I tested some ways to get this solution to work. The problem is that TestNG doesn't have an API for getting the excluded/included groups, but it does have an interception point for customization of excluded/included methods (IMethodInterceptor). So instead of having a user exclude the JAKARTAEE_FULL group, we would instead want to have them configure a system property, in my example provided below I used jakarta.run.full.profile. That way we can intercept TestNG and automatically exclude these tests AND also have a way to dynamically produce a deployment archive for either web or full profile.

Personally, I think going this route makes the TCK a bit harder to read/understand which will affect serviceability. Ultimately, I think it would be better if we split the test class into two; one for a full profile and one for a web profile. Though I understand that might go against the service release rules.

PR showing how to make this work: jakartaredhat#1

NOTE: I just removed all servlets with an EJB reference. It may be possible to keep some of these servlets I just did this as a proof of concept and got it working.

@KyleAure Thanks so much for investigating and figuring out how to get the second option to be workable. I agree that the first option (split into one class for full and one for web profile) would be simpler and preferable. I only suggested the second option as an approach that could be valid for a service release.

This gives us 2 valid options for including the tests of the new Concurrency 3.0 function in the web profile TCK, one of which is simpler but could require an exception to be included in a service release and the other, more complex solution I expect is valid in a service release without any exception to process. I'll leave it in the hands of the author of this pull, @starksm64 , to figure out which option to take for the service release to get web profile tests working. If you go with the first option and are able to get the exception, it should be very straightforward to just duplicate those 4 test classes and have one each for full and web profiles. If you go with the second option, but find Kyle's demonstrated solution too complex, then just merge this pull without the full profile excludes portion and then Kyle can create a follow-on pull applying the the second option.

I'm fine with either way. If we need to go with the second option for EE 10, we could always switch back to the cleaner two-classes approach in EE 11.

Emily-Jiang · 2022-07-17T15:01:06Z

The .war should be executable under both under platform and web profile. If we do the second one, it seems that we force .ear is used all the time instead of .war. Maybe just convert these tests to use .war but only keep some small portions use .ear.

njr-11 · 2022-07-18T18:13:21Z

The .war should be executable under both under platform and web profile. If we do the second one, it seems that we force .ear is used all the time instead of .war. Maybe just convert these tests to use .war but only keep some small portions use .ear.

It's not that we are adding unnecessary usage .ear here - it already was that way. I like your suggestion as something to consider for EE 11, but that level of refactoring is beyond this scope of this pull, which is aiming to make minimal updates to get the tests running and have sufficient coverage in web profile so that we can release EE 10.

arjantijms · 2022-07-18T20:47:52Z

As this has received 2 approvals and has been open for 12 days, let's merge it. Additional concerns can be addressed in follow-up PRs if needed.

arjantijms · 2022-07-27T20:52:15Z

@starksm64 @Emily-Jiang @njr-11 and others

Has anyone actually tried to run the PR against GF?

It doesn't seem to actually exclude anything. I tried a number of options, but it always runs 148 tests.

E.g. I updated the runner to contain

<excludedGroups>eefull</excludedGroups>

as-in:

            <plugin>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>3.0.0-M7</version>
                <configuration>
                    <argLine>-Xmx768m</argLine>

                    <!-- Surefire / TestNG Properties -->
                    <!-- The suite, the exclude and the test dependencies together determine which tests are being run -->
                    <suiteXmlFiles>
                        <suiteXmlFile>suite.xml</suiteXmlFile>
                    </suiteXmlFiles><excludedGroups>eefull</excludedGroups>
                    <dependenciesToScan>
                        <dependency>jakarta.enterprise.concurrent:jakarta.enterprise.concurrent-tck</dependency>
                    </dependenciesToScan>
                    <properties>
                        <property>
                            <name>surefire.testng.verbose</name>
                            <value>1</value>
                        </property>
                    </properties>
                    <forkMode>once</forkMode>
                    
                    <!-- System Properties -->
                    <systemPropertyVariables>
                        <glassfish.home>${glassfish.root}/glassfish7</glassfish.home>
                        <glassfish.enableDerby>true</glassfish.enableDerby>
                        <glassfish.maxHeapSize>2048m</glassfish.maxHeapSize>
                        <glassfish.systemProperties>
                            jimage.dir=${project.build.directory}/jimage
                        </glassfish.systemProperties>
                        
                        <glassfish.postBootCommands>
                            create-file-user --groups staff:mgr --passwordfile ${project.build.directory}/test-classes/j2ee.pass j2ee
                            create-file-user --groups Manager --passwordfile ${project.build.directory}/test-classes/javajoe.pass javajoe
                        </glassfish.postBootCommands>
                    </systemPropertyVariables>
                </configuration>
            </plugin>

Any idea? Am I missing something obvious? Scott, how did you initially got it to run less tests as in the description of this issue? The above works for the CDI TCK, but not for the concurrency one seemingly.

KyleAure · 2022-07-27T21:07:38Z

@arjantijms If you specify a <suiteXmlFiles> element then the surefire plugin will ignore the <excludedGroups> element.
Here is the doc: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#excludedGroups

This parameter is ignored if the suiteXmlFiles parameter is specified.

Instead, you will need to edit your suite.xml file to contain:

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

arjantijms · 2022-07-27T21:27:11Z

@KyleAure thanks for the hint. What a super "convenient" feature to ignore that parameter, especially when you want to dynamically exclude tests based on profile or system properties.

For some reason though it does seem to work here, but maybe we all just overlooked it, and it just happened to work by accident:

https://github.com/eclipse-ee4j/glassfish/blob/master/appserver/tests/tck/cdi/cdi-full/pom.xml#L380

KyleAure · 2022-07-27T21:48:04Z

Ahh, I think the difference between the two is that we are using the surefire plugin, and the CDI TCK is using the failsafe plugin. Though I would have assumed they would work the same. I couldn't find any documentation about how the failsafe plugin handles excludedGroups

Alternative fix for jakartaee#244

5883853

Signed-off-by: Scott M Stark <starksm64@gmail.com>

arjantijms approved these changes Jul 6, 2022

View reviewed changes

Remove commented out code

04b73de

Signed-off-by: Scott M Stark <starksm64@gmail.com>

Change XRemote to XInterface to avoid future confusion

d1a608c

Signed-off-by: Scott M Stark <starksm64@gmail.com>

KyleAure approved these changes Jul 8, 2022

View reviewed changes

njr-11 reviewed Jul 12, 2022

View reviewed changes

scottmarlow mentioned this pull request Jul 15, 2022

Compatibility certification request for Eclipse GlassFish 7.0 M8 for Full Platform 10 jakartaee/platform#514

Closed

10 tasks

arjantijms merged commit 60f6bc8 into jakartaee:master Jul 18, 2022

arjantijms mentioned this pull request Jul 25, 2022

First pass at removing remote ejb interface usage #245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative fix for #244 #250

Alternative fix for #244 #250

starksm64 commented Jul 6, 2022

arjantijms commented Jul 6, 2022 •

edited

Loading

arjantijms left a comment

starksm64 commented Jul 6, 2022

brideck commented Jul 6, 2022

KyleAure left a comment

njr-11 Jul 12, 2022

starksm64 Jul 13, 2022

Emily-Jiang Jul 13, 2022 •

edited

Loading

njr-11 Jul 13, 2022

starksm64 Jul 13, 2022

njr-11 Jul 13, 2022

njr-11 Jul 13, 2022

starksm64 Jul 13, 2022

KyleAure Jul 14, 2022 •

edited

Loading

njr-11 Jul 15, 2022 •

edited

Loading

Emily-Jiang commented Jul 17, 2022

njr-11 commented Jul 18, 2022

arjantijms commented Jul 18, 2022

arjantijms commented Jul 27, 2022

KyleAure commented Jul 27, 2022

arjantijms commented Jul 27, 2022

KyleAure commented Jul 27, 2022

Alternative fix for #244 #250

Alternative fix for #244 #250

Conversation

starksm64 commented Jul 6, 2022

arjantijms commented Jul 6, 2022 • edited Loading

arjantijms left a comment

Choose a reason for hiding this comment

starksm64 commented Jul 6, 2022

brideck commented Jul 6, 2022

KyleAure left a comment

Choose a reason for hiding this comment

njr-11 Jul 12, 2022

Choose a reason for hiding this comment

starksm64 Jul 13, 2022

Choose a reason for hiding this comment

Emily-Jiang Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

njr-11 Jul 13, 2022

Choose a reason for hiding this comment

starksm64 Jul 13, 2022

Choose a reason for hiding this comment

njr-11 Jul 13, 2022

Choose a reason for hiding this comment

njr-11 Jul 13, 2022

Choose a reason for hiding this comment

starksm64 Jul 13, 2022

Choose a reason for hiding this comment

KyleAure Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

njr-11 Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Emily-Jiang commented Jul 17, 2022

njr-11 commented Jul 18, 2022

arjantijms commented Jul 18, 2022

arjantijms commented Jul 27, 2022

KyleAure commented Jul 27, 2022

arjantijms commented Jul 27, 2022

KyleAure commented Jul 27, 2022

arjantijms commented Jul 6, 2022 •

edited

Loading

Emily-Jiang Jul 13, 2022 •

edited

Loading

KyleAure Jul 14, 2022 •

edited

Loading

njr-11 Jul 15, 2022 •

edited

Loading