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 tests for type validation #1013

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Conversation

Vankog
Copy link
Contributor

@Vankog Vankog commented Nov 5, 2024

See #1012 "[Bug] withPrefabValuesForField() throws for fields declaring interfaces and super-types".

I was a little too slow to also provide the fix, but I can still provide my tests for it.

@Vankog
Copy link
Contributor Author

Vankog commented Nov 5, 2024

Unfortunately, I had issues setting up the project structure with maven. Do you use a specific version?

Prettifier or Spotless (as in PR template) are also not working.

PS D:\ext_code\equalsverifier> mvn -version
Apache Maven 3.9.9 (8e8579a9e76f7d015ee5ec7bfcdc97d260186937)
Maven home: C:\ProgramData\chocolatey\lib\maven\apache-maven-3.9.9
Java version: 1.8.0_322, vendor: Temurin, runtime: C:\Program Files\Temurin\jdk8-hotspot-8.322.6\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

assertThat(exc.getMessage(),
allOf(containsString("should be of type List"), containsString("but are HashSet")));
}
{
Copy link
Owner

Choose a reason for hiding this comment

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

I've never seen this use of blocks before. Is this a JUnit 5 thing? Will it run the second block even if the first one fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad you ask. :-)
No it is simple Java syntax.
While it makes not much sense to use them in production code, I found benefits in using it in test code.

The benefits are the scoped variables. exc is only scoped inside the brackets. Thus, you can reuse the same name in another bracket, without having to fear copy paste errors, because you may have used exc1, exc2 etc. (did happen to me more than once ;-) )

Further, it separates the cases visually.

It is something I came across a few months ago and found suitable use case for this with certain test setups. I think tests are a perfect fit for this if you don't want to use a separate method for each and every check.
i.e. The alternatives are: To use a separate method for each check (which is totally valid).
Or use @ParameterizedTest.
Or to be careful with the var-naming, so you don't mix up vars by accident.

Unfortunately, it does not have any impact on the test execution.
For the use case you mentioned, assertAll()is a suitable method. It executes all given assertions, even if some fail.

Copy link
Contributor Author

@Vankog Vankog Nov 6, 2024

Choose a reason for hiding this comment

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

Alright, changed it to assertAll.
for example, running the tests against the old equals code, produces these test failures:
image

@jqno
Copy link
Owner

jqno commented Nov 5, 2024

Thanks for the PR and the tests, I'll gladly accept them! If I knew you wanted to contribute the fix yourself, I would have waited 😄

Regarding Prettier, it should just work out of the box. But I admit I've only ever tested it on Linux and macOS, so it's interesting to know that it might not work on Windows. Can you give me the full error message that you get?
Prettier is, unfortunately, a Javascript tool, so it will install node behind the scenes. I'm planning to move away from Prettier soon-ish for this reason, but haven't had the time yet to actually do so.

@jqno
Copy link
Owner

jqno commented Nov 5, 2024

I see now that I never updated the PR template when I switched to another Maven plugin for Prettier. The command to use (for now) is mvn prettier:write. Hopefully that works for you!

@Vankog
Copy link
Contributor Author

Vankog commented Nov 5, 2024

Thanks for the PR and the tests, I'll gladly accept them! If I knew you wanted to contribute the fix yourself, I would have waited 😄

Don't worry, I was just tinkering. After all it was just a 1-liner. ;-)

The prettier error is:

PS D:\ext_code\equalsverifier> mvn prettier:write
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Build Order:
[INFO] 
[INFO] EqualsVerifier | parent                                            [pom]
[INFO] EqualsVerifier | core                                              [jar]
Downloading from central: https://repo.maven.apache.org/maven2/org/codehaus/mojo/maven-metadata.xml
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-metadata.xml
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-metadata.xml (14 kB at 25 kB/s)
Downloaded from central: https://repo.maven.apache.org/maven2/org/codehaus/mojo/maven-metadata.xml (21 kB at 36 kB/s)
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for EqualsVerifier | parent 3.17.3-SNAPSHOT:
[INFO] 
[INFO] EqualsVerifier | parent ............................ SKIPPED
[INFO] EqualsVerifier | core .............................. SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.360 s
[INFO] Finished at: 2024-11-05T16:25:08+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] No plugin found for prefix 'prettier' in the current project and in the plugin groups [org.apache.maven.plugins, org.codehaus.mojo] available from the repositories [local (C:\Users\dmoench\.m2\repository), central (https://repo.maven.apache.org/maven2)] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/NoPluginFoundForPrefixException

I have Node 22.9.0
and npm Prettier also.

I also tried updating the POM with the latest prettier-java.maven version 2.6.5 and updated the <nodeVersion> to match mine. But to no avail.

@Vankog Vankog force-pushed the 1012-exact_type_check branch from b36e1e4 to e6711e9 Compare November 5, 2024 15:52
@Vankog
Copy link
Contributor Author

Vankog commented Nov 5, 2024

OK, I used the npm version prettier-plugin-java directly via npx prettier --write "**/*.java".

However, it changed quite some other files, too. So I am not very confident that the maven version gives the same result. Maybe that's caused by the version difference of Prettier and the plugin.

@Vankog
Copy link
Contributor Author

Vankog commented Nov 5, 2024

He....
I found the issue.
After telling maven to use my JDK21, instead of my JAVA_HOME as runtime, it worked.
JDK 8 is seemingly not supported by some dependency anymore.

The good message is: The file output was the same. So it should be conforming now.
The not-so-good:
I had to adapt the endOfLine config as it conflicted with my git config (checkout CRLF, commit as LF).
But running prettier converts all my line endings to LF again, marking them as touched.
I'll create another PR with my suggestion on how to fix this.

EDIT: #1015

@jqno
Copy link
Owner

jqno commented Nov 5, 2024

Sorry, I was unable to respond sooner, or I'd have told you not to bother with Prettier. It's such a hassle to set up, with node and the specific version you have to install in order to use it (the most recent version makes a lot of unnecessary changes, so I prefer sticking with the older version for now). But good on you that you figured it out anyway!

Turns out that the build doesn't fail on Prettier, but on Checkstyle, unfortunately it doesn't like the nested scope in the test. Thanks for explaining the reasoning behind that. But I think I agree with Checkstyle on this one. Whenever I do more than one assert per method, I prefer to use assertAll() because that way, a failing test can't hide behind another failing test. I've been bitten too often by fixing an assertion, only to find out that the next assertion in the test also fails but for a different reason.

@Vankog
Copy link
Contributor Author

Vankog commented Nov 5, 2024

Sure, I'll update it tomorrow.

See jqno#1012 [Bug] withPrefabValuesForField() throws for fields declaring interfaces and super-types
@Vankog Vankog force-pushed the 1012-exact_type_check branch from e6711e9 to 31d92e2 Compare November 6, 2024 11:14
@jqno jqno merged commit 9534b24 into jqno:main Nov 6, 2024
5 of 8 checks passed
@jqno
Copy link
Owner

jqno commented Nov 6, 2024

Thanks for these tests! I might tweak them a little bit further to fit in more with the other tests, but they're certainly helpful.

@Vankog Vankog deleted the 1012-exact_type_check branch November 6, 2024 12:27
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