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

fast-track: Add a TCK test to verify that the default ExceptionMapper can be overridden. #1235

Merged

Conversation

WhiteCat22
Copy link
Contributor

The nomapper tests seem to test the default ExceptionMapper, however, we could use a test to verify that the default ExceptionMapper can be overridden.

This test overrides the default ExceptionMapper to return status HTTP 512 instead of HTTP 500 and the test client verifies that status HTTP 512 is returned.

import ee.jakarta.tck.ws.rs.common.JAXRSCommonClient;
import ee.jakarta.tck.ws.rs.lib.util.TestUtil;
import jakarta.ws.rs.core.Response.Status;
Copy link
Contributor

Choose a reason for hiding this comment

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

Change
Copyright (c) 2012, 2020, 2021
to
Copyright (c) 2012, 2024

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I was incorrect. The practices for Eclipse projects are different. https://www.eclipse.org/projects/handbook/#ip-copyright-headers

So
Copyright (c) 2012, 2020, 2021
should really be changed to
Copyright (c) 2012

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange, the document used to allow for a single year, or two years (creation, last modification), and in that manner the maven copyright plugin checks two year-values in the copyright header. I am not a lawyer, but I saw somewhere that a copyright wears off after some ten years, unless updated with a new code (and year).

Copy link
Contributor

Choose a reason for hiding this comment

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

From the same document, FAQ section:

Do we need to specify a range of years in the copyright statement?

No. In the past, legal advice was that the year of the initial creation of the content and the year of the last change should be reflected in the copyright header. This is no longer the case. Specify the year that the content was initially created in the copyright statement.

@jim-krueger jim-krueger changed the title Add a TCK test to verify that the default ExceptionMapper can be overridden. fast-track: Add a TCK test to verify that the default ExceptionMapper can be overridden. Mar 13, 2024
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.

Minus the two comments, this looks good to me. Once the changes are made I'll approve it.

@spericas
Copy link
Contributor

@WhiteCat22 Could you address the comments pls?

@WhiteCat22
Copy link
Contributor Author

I have some failures locally in the other related tests. So I don't think I can re-use this existing application for this test.

@jim-krueger jim-krueger added this to the 4.0 milestone Mar 25, 2024
@WhiteCat22 WhiteCat22 force-pushed the override_default_exception_mapper_tck branch 5 times, most recently from fda8daa to 6aee4c2 Compare March 26, 2024 21:20
@WhiteCat22
Copy link
Contributor Author

I broke out the test into a new test and application to avoid breaking existing tests.

@WhiteCat22 WhiteCat22 force-pushed the override_default_exception_mapper_tck branch 2 times, most recently from c0bf9e6 to 498aa83 Compare March 26, 2024 23:23
@spericas
Copy link
Contributor

@alwin-joseph Could you verify the new version?

jamezp
jamezp previously approved these changes Mar 27, 2024
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.

The two minor notes should not hold up merging this PR.

@alwin-joseph
Copy link
Contributor

@alwin-joseph Could you verify the new version?

New test passes with Glassfish8.

…ridden.

Signed-off-by: Adam Anderson <atanderson9383@gmail.com>
@WhiteCat22 WhiteCat22 force-pushed the override_default_exception_mapper_tck branch from 498aa83 to 5d825a4 Compare March 27, 2024 15:45
@WhiteCat22
Copy link
Contributor Author

Addressed James' comments. Also, I tested this against OpenLiberty and it passes.

@WhiteCat22
Copy link
Contributor Author

@alwin-joseph Thank you!

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.

Looks good, thank you @WhiteCat22. Passes on WildFly as well.

@spericas spericas merged commit 0cc7896 into jakartaee:main Mar 28, 2024
3 checks passed
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.

6 participants