-
Notifications
You must be signed in to change notification settings - Fork 17
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
Start ngrok if the current listened port is not the expected one #55
Conversation
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.
First of all, big thanks for contributing! Code changes are fine to me, I put 2 small comments but they are more like ideas to discuss :)
src/main/java/ngrok/NgrokRunner.java
Outdated
log.info("Ngrok was already running! Dashboard url -> [ {} ]", ngrokApiClient.getNgrokApiUrl()); | ||
tunnels = ngrokApiClient.fetchTunnels(port); | ||
} else { | ||
NgrokTunnel httpTunnel = ngrokApiClient.startTunnel(port, "http", "springboot-http-" + 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.
maybe its a good idea to make this prefix (I mean onlyspringboot-
part) configurable, or taking it from spring.application.name
property with springboot-
as a default value? wdyt?
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.
Using spring.application.name
is an excellent idea
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.
Done
.thenReturn(Collections.singletonList(tunnel)); | ||
} | ||
|
||
when(ngrokApiClient.startTunnel(anyInt(), anyString(), anyString())) |
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.
then this test could use some more serious values :)
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.
I didn't want to make too much complexity in mocks.
But to be honest I was a bit lazy too.
Do you have any values you'd like?
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.
I meant something like:
when(ngrokApiClient.startTunnel(eq(8080), eq("http"), anyString()))
.thenReturn(ngrokTunnelsList.getTunnels().get(0));
when(ngrokApiClient.startTunnel(eq(8080), eq("https"), anyString()))
.thenReturn(...
but you can leave it like this, because I got work in progress done in NgrokApiClient in order to cover full ngrok api, just need to add some tests so anyways I'll need to update this one :)
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.
I will let you do better in the next step ;)
Fyi, I didn't make real-life tests, only unit tests with mocks. |
Basically I don't have any particular way before making new release. I just test manually if new functionality works with dummy app, and when I touched some other part I test also that. Then github checks are testing if minimal configuration works with |
Ok, so feel free to ping me if something is going wrong with my changes :) |
Hi, I made tests on real app and it turns out fyi, what I did - eb29737 :) |
Awesome! I can't wait to test it. |
For #51