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

fast-track: Add multipart EntityPart tests #1230

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

jim-krueger
Copy link
Contributor

@jim-krueger jim-krueger commented Mar 9, 2024

Fixes #962

@jim-krueger jim-krueger changed the title Add multipart EntityPart tests fast-track: Add multipart EntityPart tests Mar 10, 2024
@jim-krueger jim-krueger requested a review from jamezp March 10, 2024 20:43
jamezp
jamezp previously approved these changes Mar 10, 2024
Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Contributor

@spericas spericas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the styling in the recently added TCKs in detail, but the general indentation and styling in this PR seems very different to the rest of the codebase. Any chance we can make this more consistent?

@jim-krueger
Copy link
Contributor Author

@spericas I changed tabs to spaces. Was there other style or formatting issues? Thanks

jamezp
jamezp previously approved these changes Mar 11, 2024
@spericas
Copy link
Contributor

@spericas I changed tabs to spaces. Was there other style or formatting issues? Thanks

Thanks. In general, I think we would prefer this:

                    EntityPart.withName("file")
                            .content("test file", xmlFile())
                            .mediaType(MediaType.APPLICATION_XML)
                            .build());

over this,

                    EntityPart.withName("file")
                    .content("test file", xmlFile())
                    .mediaType(MediaType.APPLICATION_XML)
                    .build()
                    );

in fluent calls.

However, I don't know how consistent we are on this. Maybe we can clean this up and set up some better checkstyle rules after the 4.0 release.

@spericas spericas requested a review from jansupol March 12, 2024 13:35
@spericas
Copy link
Contributor

@jansupol Could you also review this one?

@jim-krueger
Copy link
Contributor Author

@spericas Agreed, I will make those changes. Also, I'm working through an issue related to the handling of Headers that will likely result in some additional changes.

jamezp
jamezp previously approved these changes Mar 12, 2024
Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and the test passes on RESTEasy with WildFly.

@mkarg
Copy link
Contributor

mkarg commented Mar 13, 2024

To allow all vendors to run the new test without sacrificing their week schedule, it might be good to remove the "fast-track" marker. In the end, a missing test is not a bug, and I voted -1 for fast-tracking non-bugs.

@jim-krueger
Copy link
Contributor Author

From the original Committer Conventions:
(2) For non-API, non-spec, non-javadoc changes (e.g., pom, travis, checkstyle, etc) a proposal to fast track the PR is requested at submission time. If a fast tracked PR receives 3 committer +1 votes (and no -1), it can be merged immediately provided at least 1 day has passed since its submission.

This is a TCK change and therefore "non-API, non-spec, and non-javadoc" so it can be fast-tracked. All venders would only be required to run with this after it has been placed in a release and we are weeks (or more) away from that. That being said, this PR is generating enough discussion that it is unlikely to be merged in a fast-tracked fashion.

@jim-krueger
Copy link
Contributor Author

@jamezp @spericas @jansupol @mkarg Are there any more thoughts/concerns about the contents of this PR? Thanks

@spericas
Copy link
Contributor

In general it LGTM but I'll defer to @jansupol on this one

Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@spericas spericas merged commit 4a3ca8c into jakartaee:main Mar 20, 2024
3 checks passed
@jim-krueger jim-krueger added this to the 4.0 milestone Mar 25, 2024
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.

TCK test for multipart/form-data API
5 participants