search

jakartaee / jsonp-api

Jakarta JSON Processing
https://eclipse.org/ee4j/jsonp
Other
138 stars 59 forks source link

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

Closed lukasj closed 2 years ago

lukasj commented 2 years ago

fixes #376 fixes #377 fixes #378

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

tabishop commented 2 years ago

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

lukasj commented 2 years ago

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

tabishop commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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