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

Add Records+JsonAnySetter workarounds shared in #3439, as test cases. #4633

Closed

Conversation

yihtserns
Copy link
Contributor

@yihtserns yihtserns commented Jul 22, 2024

2 out of 3 workarounds no longer work, but I still added them anyway for awareness. UPDATE: Removed them.

@cowtowncoder
Copy link
Member

Ok, as per my note, I don't like the idea of test that verifies incorrect behavior.
Tests should only test correct, expected behavior: but to avoid CI failure blocking builds, such failing tests should be in test class(es) under src/test/.../failing.

@yihtserns
Copy link
Contributor Author

Should I just remove the test cases for workarounds that are no longer working? Putting them in src/test/.../failing seems inappropriate since I wouldn't expect them to pass in the future, ever.

@cowtowncoder
Copy link
Member

Should I just remove the test cases for workarounds that are no longer working? Putting them in src/test/.../failing seems inappropriate since I wouldn't expect them to pass in the future, ever.

Ah. Yes, if that's the case, let's just remove them. I was assuming they were to be resolved in future.

…'t really care even if it somehow works again in the future.
@yihtserns
Copy link
Contributor Author

yihtserns commented Jul 22, 2024

Turns out there WAS test created for Records+JsonAnySetter, but the scenario is only when @JsonAnySetter is on a constructor parameter directly, not on a Record component: https://github.com/FasterXML/jackson-databind/blob/2.18/src/test-jdk17/java/com/fasterxml/jackson/databind/records/RecordCreatorWithAnySetter562Test.java

That explains why it didn't catch the regression caused by #4627.

@yihtserns
Copy link
Contributor Author

Now that I know RecordCreatorWithAnySetter562Test.java exists, I'll rewrite-move my tests into that class.

@yihtserns yihtserns marked this pull request as draft July 22, 2024 14:52
@yihtserns yihtserns marked this pull request as ready for review July 22, 2024 15:00
@yihtserns
Copy link
Contributor Author

Seems like this has become a duplicate of fb82b5b.

Closing.

@yihtserns yihtserns closed this Jul 22, 2024
@yihtserns yihtserns deleted the records-jsonanysetter-tests branch July 22, 2024 22:13
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.

2 participants