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

JedisSessionDataStorageTest fails on machine that has Redis running #632

Closed
gkresic opened this issue Feb 15, 2023 · 3 comments · Fixed by #634
Closed

JedisSessionDataStorageTest fails on machine that has Redis running #632

gkresic opened this issue Feb 15, 2023 · 3 comments · Fixed by #634

Comments

@gkresic
Copy link
Contributor

gkresic commented Feb 15, 2023

Caused by JedisSessionDataStorageTest.setUpClass using default constructor of RedisServer which tries to bind to default port 6379. If that port is taken (by another Redis running), this "test server" can not be started and test fails:

https://github.com/pippo-java/pippo/blob/master/pippo-session-parent/pippo-session-jedis/src/test/java/ro/pippo/session/jedis/JedisSessionDataStorageTest.java#L39

Instead, test case should find an free port and initialize RedisServer using that port.

I'll try to submit a PR.

@mhagnumdw
Copy link
Member

mhagnumdw commented Feb 15, 2023

Maybe the pippo-test/src/main/java/ro/pippo/test/AvailablePortFinder.java class will be useful.

@gkresic
Copy link
Contributor Author

gkresic commented Feb 15, 2023

Didn't know about that one (I'm new to the project).

Adapting to be uniform, but version I used initially looks better: let OS determine free port instead of iterating through a port range. Maybe it should be used as a better implementation for AvailablePortFinder.findAvailablePort? Full disclosure: I didn't invent that new implementation, found it on Baeldung.

@mhagnumdw
Copy link
Member

@gkresic , feel free to change the implementation of the AvailablePortFinder.findAvailablePort method in a new PR.

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 a pull request may close this issue.

2 participants