search

jakartaee / rest

Jakarta RESTful Web Services
Other
361 stars 117 forks source link

[1145] Use the objectsToString() method to covert failure messages to… #1146

Closed jamezp closed 11 months ago

jamezp commented 1 year ago

… a string rather than using toString() on the object array.

resolves #1145

I don't know if this is the correct branch. If not please let me know and I can migrate it elsewhere.

spericas commented 1 year ago

@alwin-joseph Could you take a look at this one?

alwin-joseph commented 1 year ago

Since we are considering a patch release 3.1.3-1 of the TCK with added LICENCE in jar file (https://github.com/jakartaee/rest/issues/1142) , is it ok to have this change included in the same ?

@mkarg @jansupol @spericas

Otherwise this change looks fine to me.

alwin-joseph commented 1 year ago

I believe this change qualifies as "ehancement" as per https://jakarta.ee/committees/specification/tckprocess/.

Requests for improvement to tests MUST simply be created as issues with a label of enhancement in the specification project’s TCK issue tracker.

Hopefully this can be included in the next service release as the tests are not amended here.

jamezp commented 1 year ago

I believe this change qualifies as "ehancement" as per https://jakarta.ee/committees/specification/tckprocess/.

Requests for improvement to tests MUST simply be created as issues with a label of enhancement in the specification project’s TCK issue tracker.

Hopefully this can be included in the next service release as the tests are not amended here.

I'd actually disagree and consider it a bug. Right now the failure message looks something like:

[Ljava.lang.Object;@7de4bd8f ==> expected: <true> but was: <false>

This simply converts the object array into a readable string which seems to be the original intention and was just missed. The objectsToString() method was already there and the client is just invoking Object[].toString() which isn't useful.

jansupol commented 1 year ago

I'd actually disagree and consider it a bug.

if it were qualified as a bug, an exclusion would need to be created with a description of why it makes the test fail. Considering it an enhancement request, the process would be much easier, just merge the PR and have it nicely in the next release. No?

jamezp commented 1 year ago

if it were qualified as a bug, an exclusion would need to be created with a description of why it makes the test fail. Considering it an enhancement request, the process would be much easier, just merge the PR and have it nicely in the next release. No?

I guess it depends on how you look at it. The failure message is really not useful at all which IMO is a bug. The test itself is not wrong, it's just the generated message is useless. I'm fine with having this fixed in 4.0, we're past 3.1 at this point for the most part :)

alwin-joseph commented 1 year ago

If you agree we can redirect this change to master branch for 4.0 release then.

jamezp commented 1 year ago

@alwin-joseph Sure thing, I just changed the branch to release-4.0.

jamezp commented 11 months ago

Can this be merged or do we need some changes? The messages printed from the TCK aren't much good. You've got to debug a TCK test to see what the message is. For example:

assertNotNull(holder.get(), "Message not received, reconnect was done",
    cnt - 1, "times.");

Prints the following without the change:

[ERROR]   JAXRSClientIT.connectionLostFor1500msTest:591->JAXRSCommonClient.assertNotNull:768 [Ljava.lang.Object;@4674d90 ==> expected: <true> but was: <false>

The message is fairly useless.

alwin-joseph commented 11 months ago

Based on the previous discussion this is considered for 4.0 release. +1 for merging.