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

Workaround for Issue #273 #276

Merged
merged 3 commits into from
May 21, 2016
Merged

Workaround for Issue #273 #276

merged 3 commits into from
May 21, 2016

Conversation

barrantesgerman
Copy link
Contributor

@barrantesgerman barrantesgerman commented May 20, 2016

Hello

I investigated about server.join(); from the JettyServer.start() and tomcat.getServer().await(); from the TomcatServer.start()

In both cases the methods call Thread.join() and block the main thread of JUnit until the web server thread stops and release the main thread.

I searched how to make JUnit not work on the main thread, but I found nothing that would serve
I tried this and this

We must not comment server.join(); or tomcat.getServer().await(); because as AlexR says:

join() is blocking until server is ready. It behaves like Thread.join() and indeed calls join() of Jetty's thread pool. Everything works without this because jetty starts very quickly. However if your application is heavy enough the start might take some time. Call of join() guarantees that after it the server is indeed ready.

My final solution it's a workaround

JettyServer.start()

if (RuntimeMode.getCurrent() != RuntimeMode.TEST) {
    server.join();
}

TomcatServer.start()

if (RuntimeMode.getCurrent() != RuntimeMode.TEST) {
    tomcat.getServer().await();
}

According to many examples I saw on StackOverflow, in the case of JUnit you have to avoid server.join(); and as Joakim says is not needed for unit testing.

But I'm no expert in JUnit, I do not know what consequences might have

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 11.019% when pulling c102fc6 on barrantesgerman:master into 6c6d0d7 on decebals:master.

@decebals decebals changed the title -Workaround for Issue #273 Workaround for Issue #273 May 20, 2016
@decebals
Copy link
Member

@barrantesgerman Congratulation for your explanation! It's very documented. I see a little modification.
I prefer to use !pippoSettings.isTest() instead of RuntimeMode.getCurrent() != RuntimeMode.TEST. Please make the modification and I will merge this PR.
Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 11.019% when pulling e870246 on barrantesgerman:master into 6c6d0d7 on decebals:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 11.019% when pulling e870246 on barrantesgerman:master into 6c6d0d7 on decebals:master.

@decebals decebals merged commit 069d189 into pippo-java:master May 21, 2016
@decebals
Copy link
Member

Thanks!

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 this pull request may close these issues.

None yet

3 participants