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

remove tests that should of been removed for EE 10. This includes EntityBean, Container Managed Persistence, Bean Managed Persistence, Embeddable EJB Container tests #1223

Conversation

scottmarlow
Copy link
Contributor

@scottmarlow scottmarlow commented Feb 12, 2024

Remove tests that should of been removed for Jakarta EE 10 (see Removals on https://jakarta.ee/specifications/platform/10 page). This includes Entity Beans, Container Managed Persistence, Bean Managed Persistence, embeddable EJB Container tests.

…ityBean, Container Managed Persistence, Bean Managed Persistence, Embeddable EJB Container tests

Signed-off-by: Scott Marlow <smarlow@redhat.com>
@scottmarlow scottmarlow force-pushed the remove_tests_entitybeans_containmanagedpersistence_beanmanagedpersistence_embeddableejbcontainer branch from d5de093 to a6e8720 Compare February 12, 2024 19:42
@scottmarlow scottmarlow marked this pull request as ready for review February 13, 2024 15:13
@brideck
Copy link
Contributor

brideck commented Feb 14, 2024

There are some packages in the EJB bucket that contain both entity beans and other bean types, e.g. bb.localaccess.mdbqaccesstest. Don't we need to update the DDs, other beans, and test client in packages like that so as not to call the tests that are expecting the entity beans to exist?

@scottmarlow
Copy link
Contributor Author

There are some packages in the EJB bucket that contain both entity beans and other bean types, e.g. bb.localaccess.mdbqaccesstest. Don't we need to update the DDs, other beans, and test client in packages like that so as not to call the tests that are expecting the entity beans to exist?

I missed updating com.sun.ts.tests.ejb.ee.bb.session.stateless.argsemantics.Client class that has an unused String testName = "EntityBeanTest" which I agree can also be deleted.

From what I recall, I did update some classes that contained entity beans and other bean types to no longer refer to the entity beans.

@brideck
Copy link
Contributor

brideck commented Mar 4, 2024

There are some packages in the EJB bucket that contain both entity beans and other bean types, e.g. bb.localaccess.mdbqaccesstest. Don't we need to update the DDs, other beans, and test client in packages like that so as not to call the tests that are expecting the entity beans to exist?

I missed updating com.sun.ts.tests.ejb.ee.bb.session.stateless.argsemantics.Client class that has an unused String testName = "EntityBeanTest" which I agree can also be deleted.

From what I recall, I did update some classes that contained entity beans and other bean types to no longer refer to the entity beans.

I brought it up because I looked at the commit and it seemed inadequately done. The 4 bb.localaccess packages that contain both entity beans and other 2.x bean types merely had the entity bean classes ripped out without adjusting test classes or metadata files to account for their absence.

I haven't looked at all of these, but this is the list of packages that contain both entity beans and other 2.x bean types and should be double-checked and updated as needed:

bb.localaccess.mdbqaccesstest
bb.localaccess.mdbtaccesstest
bb.localaccess.sbaccesstest
bb.localaccess.webaccesstest
bb.session.stateful.argsemantics
bb.session.stateless.argsemantics
deploy.mdb.ejblink.single
deploy.mdb.ejblink.singleT
deploy.mdb.ejbref.single
deploy.mdb.ejbref.singleT
deploy.session.stateful.ejblink.single
deploy.session.stateful.ejbref.single
deploy.session.stateless.ejblink.single
deploy.session.stateless.ejbref.single
timer.mdb
timer.session.stateless.cm

Note: This may be moot depending on whether or not other EJB 2.x tests also need to be removed from the TCK.

@scottmarlow
Copy link
Contributor Author

Thanks @brideck!

@brideck
Copy link
Contributor

brideck commented Apr 26, 2024

It only occurs to me now... Is removing these tests really what we want to be doing? Aren't these valid for implementations that choose to include these optional features? Or was the plan to move them somewhere else before EE 11 completes?

@scottmarlow
Copy link
Contributor Author

scottmarlow commented Apr 30, 2024

It only occurs to me now... Is removing these tests really what we want to be doing? Aren't these valid for implementations that choose to include these optional features?

The Jakarta EE 10 Platform already removed the technologies mentioned in the title. You proposed converting the EJB 2.x tests to EJB 3 which sounds fine. If you are still planning to do that, this pull request should be changed to draft which I will do now.

Or was the plan to move them somewhere else before EE 11 completes?

On the EE 11 critical path is refactoring the Platform TCK tests which does not include moving removed technolgy tests to somewhere else. I think WildFly, Open Liberty and others want to have removed tests still kept somewhere but I'm not yet sure of who will work on that.

Fyi, I think jakartaee/jakartaee-api#162 is removing other optional technologies for EE 11 if I understand correctly.

@scottmarlow scottmarlow marked this pull request as draft April 30, 2024 15:00
@brideck
Copy link
Contributor

brideck commented Apr 30, 2024

And... it just let me edit Scott's comment. That is not what I intended to do. Sorry about that.

The Jakarta EE 10 Platform already removed the technologies mentioned in the title. You proposed converting the EJB 2.x tests to EJB 3 which sounds fine. If you are still planning to do that, this pull request should be changed to draft which I will do now.

Ah, I'm getting confused between Platform and individual component specifications. These were removed from platform in EE 10, but remain an optional part of EJB 4.0. I would think that means that we would want to eventually move them to some sort of standalone EJB TCK as optional tests.

My EJB 2.x -> 3 work actually requires this PR as a prereq, so we'll need to put this in as well. I'll get to work on reviewing it this week and see if I can spot any more changes that might be needed.

@scottmarlow
Copy link
Contributor Author

And... it just let me edit Scott's comment. That is not what I intended to do. Sorry about that.

I didn't know either that could happen. Weird.

The Jakarta EE 10 Platform already removed the technologies mentioned in the title. You proposed converting the EJB 2.x tests to EJB 3 which sounds fine. If you are still planning to do that, this pull request should be changed to draft which I will do now.

Ah, I'm getting confused between Platform and individual component specifications. These were removed from platform in EE 10, but remain an optional part of EJB 4.0.

+1 on these technologies being optional in the EJB 4.0 spec.

I would think that means that we would want to eventually move them to some sort of standalone EJB TCK as optional tests.

I think that we need to move them somewhere out of the Platform TCK repo but that doesn't have to be done with an EE 11 branch as the source. We could instead copy the tests from an EE 10 branch as well (or even EE 8 if wanted to copy the interop tests).

My EJB 2.x -> 3 work actually requires this PR as a prereq, so we'll need to put this in as well. I'll get to work on reviewing it this week and see if I can spot any more changes that might be needed.

Thanks, I didn't know that you were building off of this pull request. :-) I'll convert it back to non-draft.

@scottmarlow scottmarlow marked this pull request as ready for review April 30, 2024 16:33
Copy link
Contributor

@brideck brideck left a comment

Choose a reason for hiding this comment

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

I believe this is looking good now, but would appreciate another set of eyes, if anyone is willing.

Also, if anyone disagrees with the tests I've brought back in my newer commits, please let me know. As far as I can see, they do not contain any Entity EJBs or Embeddable Container tests.

Copy link
Contributor Author

@scottmarlow scottmarlow left a comment

Choose a reason for hiding this comment

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

Interesting change, pull request authors can't approve their own pull request.

@scottmarlow scottmarlow merged commit 6742ec7 into jakartaee:tckrefactor May 2, 2024
1 of 2 checks passed
@scottmarlow
Copy link
Contributor Author

Thanks @brideck for your updates! If anyone does agree with any of these changes, a new pull request is the best way to make corrections.

@scottmarlow scottmarlow deleted the remove_tests_entitybeans_containmanagedpersistence_beanmanagedpersistence_embeddableejbcontainer branch June 14, 2024 14:11
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.

2 participants