Best practice unit test module structure #108
Conversation
There was a problem hiding this comment.
I had a hard time finding an example or documentation on the best practice here. I followed an example from https://github.com/apache/maven-surefire.git under maven-multimodule-project-with-jpms.
|
Yes, we spent an inordinate amount of time trying to get this right for my book. I'm still not sure that my suggestion is the best way, but it feels so, based on the projects that I know have spent a lot of time thinking about this (Jetty & OpenJDK) |
|
The code changes are fine at the micro level. It does feel a little bit awkward to have to introduce --opens syntax just to run tests though. I wonder if we should run this via jigsaw-dev for their opinion? |
|
That's why we have --add-opens etc. |
Not as awkward as the code it replaces. |
|
The thinking is basically that we don't want our shipped modules (api, parser, etc.) to have any reference to our tests. The tests are external to that code and once we have built and tested our modules, then we ship them without the tests (of course). |
|
The testing practice followed was best practice a few years ago.. following guidelines set by Kevlin Henny. The thinking is that by not having tests in the same package you are forced to test via public APIs. This tends to keep the tests away from implementation details which may offer an impedance to refactoring.. and indeed, this is something that we have experienced in the past, having large number of tests need to be modified and expediency just having them removed with the idea that they would be added back in... I'd call that a failure. Thing is, the tests that seemed to have survived were those that didn't muck around with the internals where as those that did died in the todo: list. As it stands, there is nothing that I know of in the code base that is there specifically to support tests. Well, I lie... sort of, there is one method that I use to help debug the parse rules.. but it's not used in testing. I'm all for removing test code from modules.. however, I think we should have a deeper think about all the implications backing away from tests in a different package structure. |
|
I know of at least one bug in your code due to inadequate tests, not
counting the one I reported with the broken compareTo method.
…On Mon, 27 Sep 2021 at 21:52, Bruno Borges ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#108 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACRE3VNINPJGZWNZOBTZY3UEC4PFANCNFSM5E2E2ODQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I couldn't quite tell from @kirk-microsoft message whether he thinks this PR is OK, but I'm assuming since there's no requested changes in his review we can land this. |
|
Sorry but I didn't ok this PR.. I was trying to sort out what best practices are and how they might be applied to this project given that there is a difference of opinion here. |
|
Ah apologies - let's use the Review and Request changes option next time so there's a clear -1 on the PR until we're aligned. |
|
@kirk-microsoft I just tested the project on VS Code, IntelliJ, and Eclipse, and I was finally able to have a proper developer environment with full IDE capabilities. This was only possible with this change. I believe this is the right path and wouldn't go back to any of the previous structures. PS: only NetBeans still has some problems. |
|
@kirk-microsoft what is the problem of having tests in the same package of the classes they are testing? |
|
I don't know of any well-known OSS project out there that has the test classes in separate packages. Just checked now and that's even how JUnit does it: https://github.com/junit-team/junit5/tree/main/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine It's how Jetty is set up. Spring Boot seems to do the same: https://github.com/spring-projects/spring-boot/tree/main/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate I would not be surprised if you heard Kevlin Henney say this in a talk - he's a great philosopher and speaker. And he is correct on quite a few levels. However, by doing things differently we are going against what everyone else is doing, causing confusion with the IDEs and tools out there. We certainly shouldn't have our core modules export their packages to the test modules, that feels wrong. |
Following what I believe are "best practices" for JPMS, I believe that the unit tests should not be in a module, and should be in the same package as what is being tested. Furthermore, the module-info.java file should not refer to test modules. I base this belief that this is the best practice on the OpenJDK/jdk not exporting to test modules in any of their modules. I also base it on my own experience in creating the framework for my Dynamic Proxies in Java book samples. When I created those sample, I had input from folks from Jetty and Maven, and I believe that my structure is a better approach than what is currently used in gctoolkit. Lastly, I looked at the structure of Jetty this morning, particularly the server module and that also follows the same structure I am proposing here.
Note that the module-info.java files now only export to other non-test modules in our project. Where we need to access non-exported packages, we do so with an added --add-opens inside the pom.xml file. This also follows best practices of the Jetty project and my book samples.