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

FreePort resource to create ServerSocket #733

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Michael1993
Copy link
Member

Another approach to #141. Based on #541

@Michael1993 Michael1993 requested a review from beatngu13 May 2, 2023 13:51
@Michael1993
Copy link
Member Author

A couple things that need to be updated still:

  • documentation has to be updated
  • add @NewPort/@SharedPort annotation (?)

@Michael1993 Michael1993 requested a review from Bukama May 10, 2024 14:53
@Bukama Bukama added this to the Pioneer 2.3.0 milestone May 10, 2024
Copy link
Member

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

Aside from my thought that I might have an understanding issue personally, please add some Javadoc for the class and interface (and maybe answer my personal questions :) )

to test a failure scenario when there is no service at given port.

It's a JUnit Jupiter extension which provides a free port for above-mentioned use cases.
Also, there is no guarantee that the port will still be available by the time you use it.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe highlight this (port might be not free when used) as a note / warning?

import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@New(FreePort.class)
Copy link
Member

Choose a reason for hiding this comment

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

I think it means that a new instance of the FreePort class is created, but I would like to double check as I know @New only as a deprecated CDI annotation (and I'm not getting any other proper search result from google 🆘). This would mean that the user always get a Socket on port zero - which is hardly what they want I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again - NewPort was removed. The @New annotation is part of our resources abstract extension.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for updating.

@Michael1993
Copy link
Member Author

I intend to tackle #808 in this one when I have the time, actually.

@Bukama
Copy link
Member

Bukama commented May 14, 2024

Looks good to me, @beatngu13 would you also take a look?

@Michael1993 It's still a draft? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants