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

#376: JsonProviderTest fails if run after other Junits in a test suite #380

Merged
merged 5 commits into from Jul 15, 2022
Merged

Conversation

lukasj
Copy link
Contributor

@lukasj lukasj commented May 19, 2022

fixes #376
fixes #377
fixes #378

@KyleAure/@tabishop , @starksm64 - can you check this fixes issues you are seeing, please?

Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
@tabishop
Copy link

This on its own doesn't fix the problem because the JsonProvider code checks to see if the class has been instantiated before it tries to get the system property. If the class has already been instantiated, it doesn't even look at the system property again so resetting it has no effect.

https://github.com/eclipse-ee4j/jsonp/blob/master/api/src/main/java/jakarta/json/spi/JsonProvider.java#L103L105

Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
@lukasj
Copy link
Contributor Author

lukasj commented May 19, 2022

Now I know which static variable you were referring to. Try again, please.

Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
@tabishop
Copy link

So looks like your latest change, changes the behavior of the JsonProvider class slightly in that previously the System property would only be read once in the static code block and with your change it would be read every time a provider instance is requested.

The spec doesn't say specifically if this new usage scenario should be supported: https://jakarta.ee/specifications/jsonp/2.1/apidocs/jakarta.json/jakarta/json/spi/jsonprovider#provider()

The behavior change is subtle, but wondering if changing the behavior to get the test to pass is the correct resolution as opposed to removing the test from the TCK?

@lukasj
Copy link
Contributor Author

lukasj commented May 19, 2022

The spec doesn't say specifically if this new usage scenario should be supported

Neither it says the opposite. All it says is if the property is set, it is used (+ 2 more steps) and that it is a responsibility of the user to cache the actual provider

The motivation for the original code was to not allow one deployment to influence the other one - imagine there are two apps on the server using the API, one decides to call setProperty for some reason and the other one does not expect it

wondering if changing the behavior to get the test to pass is the correct resolution as opposed to removing the test from the TCK?

simple addition of Each test must be run in its own JVM. requirement to the TCK User guide also solves the problem, doesn't it?

@tabishop
Copy link

Agreed that addition of Each test must be run in its own JVM. requirement to the TCK User guide also solves the problem

@lukasj lukasj merged commit fc14a6e into jakartaee:master Jul 15, 2022
@lukasj lukasj deleted the is376 branch July 15, 2022 15:19
@lukasj lukasj added this to the 2.1.1 milestone Jul 20, 2022
@lukasj lukasj added this to Done in 2.1.1 Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2.1.1
Done
3 participants