jstl-api icon indicating copy to clipboard operation
jstl-api copied to clipboard

Tags 3.0 TCK Challenge for test ExceptionCheckTag.java depending on removed Pages 4.0 JspException.getRootCause() method...

Open scottmarlow opened this issue 1 year ago • 14 comments

JspException.getRootCause() was removed from Pages 4.0 and if a Tags 3.0 implementation fails certain Tags 3.0 TCK tests, the cause of failure is unknown due to https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jstl/common/tags/ExceptionCheckTag.java#L174 calling the removed JspException.getRootCause() which results in the TCK failure cause not being shown.

The combination of Tags 3.0 + Pages 4.0 Specifications are expected to be used together in Jakarta EE 11 implementations.

Tags 3.0 TCK cannot depend on Pages SPEC API methods removed in Pages 4.0. Since the Tags 3.0 TCK can instead use JspException.getCause(), the Tags 3.0 TCK should be updated to use JspException.getCause() instead of JspException.getRootCause().

Details on the failure from https://github.com/jakartaee/tags/issues/255#issuecomment-1994755183:

[javatest.batch] 01-30-2024 13:00:54: TRACE: [WIRE] - << "

Eclipse GlassFish 8.0.0 - Error report HTTP Status 500 - Internal Server Error

type Exception report

messageInternal Server Error

descriptionThe server encountered an internal error that prevented it from fulfilling this request.

exception

java.lang.NoSuchMethodError: 'java.lang.Throwable jakarta.servlet.jsp.JspException.getRootCause()'

scottmarlow avatar Mar 13 '24 20:03 scottmarlow

@scottmarlow just to set expectations, is this issue something you'd like one of the Jakarta Tags committers to complete in the platform-tck? Which branch? 10.0?

pnicolucci avatar Mar 14 '24 00:03 pnicolucci

@scottmarlow just to set expectations, is this issue something you'd like one of the Jakarta Tags committers to complete in the platform-tck? Which branch? 10.0?

Updated: I expect that one of the Platform TCK committers will create a pull request to update the tests to use getCause instead of getRootCause() and stage a Platform TCK for testing (as well as a Tags TCK).

+1 if anyone else wants to create the pull request against https://github.com/jakartaee/platform-tck/tree/10.0.x (need to update ts.jtx files + https://github.com/jakartaee/platform-tck/blob/10.0.x/release/tools/jakartaee.xml + https://github.com/jakartaee/platform-tck/blob/10.0.x/release/tools/jstl.xml)

scottmarlow avatar Mar 14 '24 12:03 scottmarlow

@scottmarlow we shouldn't exclude the tests but rather use getCause() rather than getRootCause()

pnicolucci avatar Mar 14 '24 13:03 pnicolucci

@scottmarlow we shouldn't exclude the tests but rather use getCause() rather than getRootCause()

Good point and agreed!

Updated: Note that I updated my previous comment today on March 15, 2024.

scottmarlow avatar Mar 14 '24 14:03 scottmarlow

@arjantijms @volosied @m0mus @jgallimore Given the information here any objection to accepting this challenge? I'm tagging several committers in the project to work to a consensus.

pnicolucci avatar Mar 15 '24 14:03 pnicolucci

Maling list thread: https://www.eclipse.org/lists/jstl-dev/msg00134.html

pnicolucci avatar Mar 19 '24 11:03 pnicolucci

@scottmarlow, I've accepted this challenge. Let me know if any further action is required from the Jakarta Tags team.

pnicolucci avatar Mar 22 '24 12:03 pnicolucci

We will merge the https://github.com/jakartaee/platform-tck/pull/1219 change as soon as possible that will exclude the failing tests.

scottmarlow avatar Apr 24 '24 18:04 scottmarlow

We will merge the jakartaee/platform-tck#1219 change as soon as possible that will exclude the failing tests.

@scottmarlow I thought we had agreed that these tests would be updated and not excluded: https://github.com/jakartaee/tags/issues/256#issuecomment-1997660685

pnicolucci avatar Apr 24 '24 21:04 pnicolucci

We will merge the jakartaee/platform-tck#1219 change as soon as possible that will exclude the failing tests.

@scottmarlow I thought we had agreed that these tests would be updated and not excluded: #256 (comment)

Thanks @pnicolucci I really appreciate your catching me overlooking that!

scottmarlow avatar Apr 24 '24 22:04 scottmarlow

@scottmarlow were these tests ever updated or do you need help with a PR? Do we need to remove the exclusions: https://github.com/jakartaee/platform-tck/pull/1219.

pnicolucci avatar Jun 03 '24 16:06 pnicolucci

@scottmarlow were these tests ever updated or do you need help with a PR? Do we need to remove the exclusions: https://github.com/jakartaee/platform-tck/pull/1219.

@pnicolucci https://github.com/jakartaee/platform-tck/pull/1219 did not exclude the Tags tests as that part of the pull request was reverted.

Thanks for volunteering! Yes, please update the the Tags tests to use JspException.getCause() instead of JspException.getRootCause().

scottmarlow avatar Jun 03 '24 18:06 scottmarlow

@scottmarlow I should have a set of PRs up for this tonight sometime, I'm just validating all the tests pass.

pnicolucci avatar Jun 04 '24 18:06 pnicolucci

PRs are up and ready for review!

pnicolucci avatar Jun 04 '24 20:06 pnicolucci

@scottmarlow can we close this issue out?

pnicolucci avatar Aug 06 '24 16:08 pnicolucci

Thank you for your help @pnicolucci!

scottmarlow avatar Aug 06 '24 16:08 scottmarlow