search

jakartaee / rest

Jakarta RESTful Web Services
Other
361 stars 117 forks source link

fast-track: 1162 sse broadcast test #1173

Closed jim-krueger closed 1 year ago

jim-krueger commented 1 year ago

Fixes #1162

alwin-joseph commented 1 year ago

I'm not sure why we target release-3.1.x. I think this should just go to master. @alwin-joseph?

Yes, if the challenge is accepted the TCK 3.1.4(release-3.1.x branch) will have a PR to exclude the test, the fix can be done in master or 4.0.

spericas commented 1 year ago

@jim-krueger Could you update the target branch?

alwin-joseph commented 1 year ago

IIUC https://github.com/jakartaee/rest/pull/1172 has the same fix targeted to release-4.0(I assumed this is same as merging to master) which means this PR can either be converted to exclude the test in release-3.1.x or create a new PR for excluding and abandon this PR . Am I right ?

jim-krueger commented 1 year ago

Please pardon my confusion. But if a 3.1.4 version is being created, why not just include this fix rather than excluding the test?

On Aug 30, 2023, at 10:01 AM, Alwin Joseph @.***> wrote:

IIUC #1172 https://github.com/jakartaee/rest/pull/1172 has the same fix targeted to release-4.0 which means this PR can either be converted to exclude the test in release-3.1.x or create a new PR for excluding and abandon this PR . Am I right ?

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/pull/1173#issuecomment-1699350361, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQ. You are receiving this because you were mentioned.

spericas commented 1 year ago

On Aug 30, 2023, at 2:08 PM, Jim Krueger @.***> wrote:

Please pardon my confusion. But if a 3.1.4 version is being created, why not just include this fix rather than excluding the test?

Modifying a test in the same 3.1.X release may cause another implementation that is already passing the TCKs to fail. In general TCKs are disabled and fixed in a future version (dot rather than dot-dot). But Alwin is really the expert here.

— Santiago

On Aug 30, 2023, at 10:01 AM, Alwin Joseph @.***> wrote:

IIUC #1172 https://github.com/jakartaee/rest/pull/1172https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1172*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoFOF_Bg$ has the same fix targeted to release-4.0 which means this PR can either be converted to exclude the test in release-3.1.x or create a new PR for excluding and abandon this PR . Am I right ?

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/pull/1173#issuecomment-1699350361https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699350361*3E__;IyU!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofeqz2kW8$, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQhttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQ*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofOwQ41Mc$. You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699626582__;Iw!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoziV8N0$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABKT2N6TPM5LFTPEJRMIEPLXX56TDANCNFSM6AAAAAA3Y5NQXQ__;!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofwSctAcw$. You are receiving this because you commented.Message ID: @.***>

jim-krueger commented 1 year ago

I see. That makes sense but it doesn’t apply in this case. The problem that is fixed with this PR is to remove the previous order requirement for returned messages that was there before. Removing that order check could not possibly affect any implementation where the test currently works, but would keep this issue from occurring intermittently. But if Alwin or others feel strongly about this I will exclude the test in 3.1.4. Thanks

On Aug 30, 2023, at 2:00 PM, Santiago Pericas-Geertsen @.***> wrote:

On Aug 30, 2023, at 2:08 PM, Jim Krueger @.***> wrote:

Please pardon my confusion. But if a 3.1.4 version is being created, why not just include this fix rather than excluding the test?

Modifying a test in the same 3.1.X release may cause another implementation that is already passing the TCKs to fail. In general TCKs are disabled and fixed in a future version (dot rather than dot-dot). But Alwin is really the expert here.

— Santiago

On Aug 30, 2023, at 10:01 AM, Alwin Joseph @.***> wrote:

IIUC #1172 https://github.com/jakartaee/rest/pull/1172https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1172*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoFOF_Bg$ has the same fix targeted to release-4.0 which means this PR can either be converted to exclude the test in release-3.1.x or create a new PR for excluding and abandon this PR . Am I right ?

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/pull/1173#issuecomment-1699350361https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699350361*3E__;IyU!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofeqz2kW8$, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQhttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQ*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofOwQ41Mc$. You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699626582__;Iw!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoziV8N0$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABKT2N6TPM5LFTPEJRMIEPLXX56TDANCNFSM6AAAAAA3Y5NQXQ__;!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofwSctAcw$. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/pull/1173#issuecomment-1699687629, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQE4Y3VAT237EAXY463XX6EUTANCNFSM6AAAAAA3Y5NQXQ. You are receiving this because you were mentioned.

spericas commented 1 year ago

On Aug 31, 2023, at 10:42 AM, Jim Krueger @.***> wrote:

I see. That makes sense but it doesn’t apply in this case. The problem that is fixed with this PR is to remove the previous order requirement for returned messages that was there before. Removing that order check could not possibly affect any implementation where the test currently works, but would keep this issue from occurring intermittently. But if Alwin or others feel strongly about this I will exclude the test in 3.1.4. Thanks

It’s a general rule, not specific to this case. In fact, it is possible for a “fix” to introduce a new problem that wasn’t seen before (regardless of how good the fix is), so it is just safer to omit the test until the next round.

— Santiago

On Aug 30, 2023, at 2:00 PM, Santiago Pericas-Geertsen @.***> wrote:

On Aug 30, 2023, at 2:08 PM, Jim Krueger @.***> wrote:

Please pardon my confusion. But if a 3.1.4 version is being created, why not just include this fix rather than excluding the test?

Modifying a test in the same 3.1.X release may cause another implementation that is already passing the TCKs to fail. In general TCKs are disabled and fixed in a future version (dot rather than dot-dot). But Alwin is really the expert here.

— Santiago

On Aug 30, 2023, at 10:01 AM, Alwin Joseph @.***> wrote:

IIUC #1172 https://github.com/jakartaee/rest/pull/1172https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1172*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoFOF_Bg$https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1172*3E*3Chttps:/*urldefense.com/v3/__https:/*github.com/jakartaee/rest/pull/1172*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoFOF_Bg$*3E__;JSUvLyol!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2WURUGrGo$ has the same fix targeted to release-4.0 which means this PR can either be converted to exclude the test in release-3.1.x or create a new PR for excluding and abandon this PR . Am I right ?

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/pull/1173#issuecomment-1699350361https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699350361*3E__;IyU!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofeqz2kW8$https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699350361*3E*3Chttps:/*urldefense.com/v3/__https:/*github.com/jakartaee/rest/pull/1173*issuecomment-1699350361*3E__;IyU!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofeqz2kW8$*3E__;IyUlLy8qKiU!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2WxBVm2-A$, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQhttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQ*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofOwQ41Mc$https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQ*3E*3Chttps:/*urldefense.com/v3/__https:/*github.com/notifications/unsubscribe-auth/AHSLZQGS4MQ6S7IA577HY3TXX5ITBANCNFSM6AAAAAA3Y5NQXQ*3E__;JQ!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofOwQ41Mc$*3E__;JSUvLyol!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2W9xtQjmg$. You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699626582__;Iw!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofoziV8N0$%3E, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABKT2N6TPM5LFTPEJRMIEPLXX56TDANCNFSM6AAAAAA3Y5NQXQ__;!!ACWV5N9M2RV99hQ!LkOsazbnzbMElbyhdSjnRgmnZXmQFPR7jAzcGDNSZAFkJ7x_sFaqvW-6uuOp0fPGnQU9Z4_FoRLZd-0YlndL5Q8AGVofwSctAcw$%3E. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/pull/1173#issuecomment-1699687629https://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1699687629*3E__;IyU!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2WV_XOgnI$, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQE4Y3VAT237EAXY463XX6EUTANCNFSM6AAAAAA3Y5NQXQhttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AHSLZQE4Y3VAT237EAXY463XX6EUTANCNFSM6AAAAAA3Y5NQXQ*3E__;JQ!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2WZ-caobQ$. You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/jakartaee/rest/pull/1173*issuecomment-1701183607__;Iw!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2WexHkhWA$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABKT2N5HC4RK3E3O65P43QTXYCPEHANCNFSM6AAAAAA3Y5NQXQ__;!!ACWV5N9M2RV99hQ!L5PUXxE9Zk93piXn_6g0LTiKOJVRmawnaW4IZHffjYXpNcbGMwbF975_py6mG9OKI2lUappITYvrAAQXFvM4AkMcj-2W-mzbkeI$. You are receiving this because you commented.Message ID: @.***>

alwin-joseph commented 1 year ago

I see. That makes sense but it doesn’t apply in this case. The problem that is fixed with this PR is to remove the previous order requirement for returned messages that was there before. Removing that order check could not possibly affect any implementation where the test currently works, but would keep this issue from occurring intermittently. But if Alwin or others feel strongly about this I will exclude the test in 3.1.4. Thanks

Yes it is possible to fix tests in a service release as I mentioned in https://github.com/jakartaee/rest/pull/1164#issuecomment-1652867664 , referring to https://jakarta.ee/committees/specification/tckprocess/ (especially under section "Accepted Challenges").

From the process document :

"if there is already at least one certified compatible implementation before the challenge, the new, modified TCK should be run against at least one such implementation (and ideally all of them) before the changes are published, released, and finalized.

If such a change were released, and it was later found to cause a previously-certified implementation to fail the new, modified test, then excluding the test would likely be the only option, and this would require yet another, additional service release."

Not against the fix, but only looked safer and easier option to exclude the test. If the REST spec team feel that this test is vital to be not excluded in 3.1.4 we could try fixing it. Thoughts ?

It’s a general rule, not specific to this case. In fact, it is possible for a “fix” to introduce a new problem that wasn’t seen before (regardless of how good the fix is), so it is just safer to omit the test until the next round.

Agree.

jim-krueger commented 1 year ago

As requested, I have excluded/disabled the test for 3.1.4.

jim-krueger commented 1 year ago

I need a couple more approvals before I can merge this.

jim-krueger commented 1 year ago

@alwin-joseph Thanks for catching this obvious mistake! Added fast-track as well.

jim-krueger commented 1 year ago

@alwin-joseph @jansupol @jamezp Could I possibly talk one of you three into reviewing/approving this so I'd have the required 3 approvals to merge? Thanks