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

TCK Close the original stream of setEntityStream #1164

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

@spericas
Copy link
Contributor

@alwin-joseph

@jamezp
Copy link
Contributor

jamezp commented Jul 26, 2023

I can't link the issues, but this resolves #1163

@jansupol
Copy link
Contributor

@alwin-joseph @jamezp is this the right branch? Or should it be PR to master?

@jamezp
Copy link
Contributor

jamezp commented Jul 26, 2023

IMO it just depends. There definitely needs to be a master one, but if there is any chance a new 3.1.x release of the TCK would be done, I do think it makes sense to add it to this branch as well.

@alwin-joseph
Copy link
Contributor

alwin-joseph commented Jul 27, 2023

Referring to https://jakarta.ee/committees/specification/tckprocess/ what I understand is that

  • the challenge resolution is usually test exclusion.
  • But there is another option to modify the test during challenge for a service release which is explained starting :
    "As another alternative to excluding a challenged test, it may be possible to adjust the test validation logic to “expand” the validation check..". Does this fix qualify for this option.
    The idea is that the challenge resolution should not cause already certified implementations to fail the modified test. They mention "..modified TCK should be run against at least one such implementation..".

To conclude I think we have below options

  1. exclude the test for 3.1.4 and fix for 4.0.0 or
  2. fix the test for 3.1.4 , make sure already passed implementations do not fail with this change. Are there any compatible implementations other than Jersey for 3.1?

Please correct me if I made any incorrect assumptions.

@jamezp
Copy link
Contributor

jamezp commented Jul 27, 2023

fix the test for 3.1.4 , make sure already passed implementations do not fail with this change. Are there any compatible implementations other than Jersey for 3.1?

Yes, RESTEasy should be compatibile. It was accepted here #1111 and has been shipping with WildFly.

@radcortez
Copy link
Author

I'm fine if we can just exclude the test to certify and use 3.1.3. This is also RESTEAsy, but with Vert.x.

Now sure why this was not an issue before. I guess some runtimes may be closing the stream, but this is not reliable.

@alwin-joseph
Copy link
Contributor

In that case, we can redirect this PR to the master branch.
Once the challenge #1163 is accepted we will exclude the test in a new service release (3.1.4).

Thoughts ?

@spericas
Copy link
Contributor

In that case, we can redirect this PR to the master branch. Once the challenge #1163 is accepted we will exclude the test in a new service release (3.1.4).

Thoughts ?

I'm good with this plan

@alwin-joseph
Copy link
Contributor

alwin-joseph commented Sep 7, 2023

@radcortez Can you please redirect this PR to branch release-4.0 or master. Will create another change to exclude the test for 3.1.4 TCK.

@spericas
Copy link
Contributor

@alwin-joseph Until we decide how to proceed with the branches, we should target PR's such as this one to both branches.

@alwin-joseph
Copy link
Contributor

Gentle reminder, the issue is resolved by excluding the test in 3.1.4 release in #1181.
This PR should be redirected to release-4.0 and master as per previous discussion.

@radcortez radcortez changed the base branch from release-3.1.x to release-4.0 October 12, 2023 09:46
@radcortez
Copy link
Author

This PR should be redirected to release-4.0 and master as per previous discussion.

I've changed the base to release-4.0. Thanks!

@spericas spericas merged commit 33c933f into jakartaee:release-4.0 Dec 12, 2023
1 check 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.

5 participants