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
Adds FreePort test extension #541
Conversation
Hey @p1729, Thx for your PR! We will look into this as soon as possible. ✌️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR and sorry for the delay in getting a response by our team.
Please add a documentation (under /docs
folder) page which explains the (usage of) the extension and shows at least one use cases with example.
Maybe you can also write a small note in which cases this extension is different from solutions like the TestContainer
project.
@Bukama Thanks. Could you please review the new changes, as I think I have fixed most of the requested changes. |
Hi @p1729, I finally have some time to take a closer look at this. Let's see where it goes. 😃 |
After going over this PR (which looks really good, btw), I eventually noticed that there's likely some overlap with the more general resource mechanism that we're building in #348/#491. It seems to me that a port would be a fitting resource. So instead of merging this PR now and then later having to retrofit the extension to work with the new mechanism (if that makes sense), I'd rather put this on hold until we know more on that front. I'm really sorry that the time and effort you invested will not lead to a merged PR for now. You clearly went over the contribution guide and wrote good docs and I'm thankful for that. It sucks that we're just gonna let this sit for some time, but I hope you understand. :) |
@nipafx Not a problem. Keep me apprised of the things related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @p1729! This looks super.
That said, I do have a couple notes here. 🧐
Behaviour
- We generally prefer to create our own annotation instead of
@ExtendWith
. This is especially useful if you need to pass certain parameters to your extension (which we probably want to do - see next points). - In the linked issue the recommended behaviour of this extension is is to be able to designate a "strategy" class. For example an annotation with a
strategy
property is a good approach. - The extension assigns a free port number rather eagerly, i.e. when you create your
FreePort
instance - I think this is... fine? But I'd rather have another option available: only assign the port number when the user calls theget
ornumber
methods. (Could also be in the annotation aslazy = true
) - Also, why not just return with the
ServerSocket
? We can document that in this case the extension does not handle the closing and the user should use a try-with-resources statement.
Tests
The tests need to be expanded - now it basically tests the bare minimum. For example, I'd want to see it handle the port not being available with isFreeNow
.
This is a splendid first approach. You might have been waiting for #491 to be merged before expanding this (Which is still not merged... that's on us. 😬). You might also have been waiting for some detailed feedback (again, on us. 😬). Either way, I wanted to give you this feedback in hopes we will merge #491 soon (to let you start working on it as soon as you want). 🤞
import java.io.IOException; | ||
import java.net.ServerSocket; | ||
|
||
public final class FreePort { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this final
?
== Introduction | ||
|
||
Sometimes we want a free port in our tests so that we can either start a server on it or | ||
to test a failure scenario when there is no service at given port. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the "one sentence per line" rule - helps us keep our documentation simple.
We've finally merged support for resources! 🥳 You can learn all about it here: This extension should use the same mechanism, which requires a major refactoring. @p1729: Are you interested to do this? If not, that's ok, too, then somebody else will tackle it. |
@nipafx / @Michael1993 I'm interested and will refactor as per the new design soon. |
Cool! Looking forward to it. :) |
Hi again @p1729 ! Just checking in if you might want to let someone else tackle this if you don't have the time? |
@Michael1993 Sure, pls go ahead. |
I think two years after opening the PR, it's safe to close it. Sorry @p1729 that we let you down and didn't get around to including your contribution. 😔 |
Proposed commit message:
PR checklist
The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.
Documentation (general)
.adoc
file in thedocs
folder, e.g.docs/report-entries.adoc
.adoc
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode
Contributing
README.md
mentions the new contribution (real name optional)I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.