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

Issue-1325: Let OS assign random ports to servers during testing, and… #1336

Merged
merged 2 commits into from Jun 14, 2019

Conversation

@srdo
Copy link
Contributor

commented Jun 8, 2019

… let tests get the port of the running server once the OS has assigned a port. Delete PortUtil and similar utilities.

As discussed in #1325, I've updated the tests to let the OS assign ports to test servers, rather than using the PortUtil and various other utilities that were similar.

The method for assigning ports outlined in the issue works everywhere, except for the following two places:

  • RestHookWithInterceptorR4Test - Wants to set a subscription endpoint that won't respond properly. Instead of picking a random port, I changed the test to use the running test server, with an invalid URL path.
  • TestSubscriptionConfig - Due to Spring, the tests using this config need to supply an IGenericClient, that apparently isn't used. Instead of creating a real client pointing to a random port, I just made the config supply a mock. The client was previously pointing to a port where nothing was running, so I don't think this should make a difference.

I've not updated the following modules, as they aren't attached to the build. I'm not sure why they aren't deleted to be honest:

  • hapi-fhir-oauth2
  • hapi-fhir-structures-dstu
  • hapi-fhir-jaxrs-sse

I'll be happy to open a PR to delete these modules if they aren't needed.

Other changes here:

  • Some tests were closing servers in @AfterClass methods, while also clearing all static fields in a different @AfterClass, using the TestUtil.clearAllStaticFieldsForUnitTest() method. As JUnit may invoke these methods in any order, this caused servers to leak, due to the field being nulled before the server was stopped. Fixed the instances I found by just combining the methods.
  • Jetty is only capable of shutting down gracefully if the StatisticsHandler is added to the server. Made sure this happens. When Jetty shuts down gracefully, a stop() call will block until the server is done processing open requests. Without StatisticsHandler, the server just shuts down immediately, which was causing exceptions to be logged in some tests.
  • Ensured tests call both stop and destroy on the Jetty server. As far as I can tell, stop doesn't do cleanup (the server could be started again later), and destroy should be called to make sure all handlers are properly destroyed once the server is stopped.
  • Removed a ComponentScan that was trying to scan the "ca.uhn.fhir.jpa.model.interceptor.executor" package. This package doesn't seem to exist, and the tests run fine without it.

I think it would be good to write e.g. a JUnit rule to wrap server creation/destruction, so test writers won't have to remember to call the right test utility methods, and to properly clean up the server. I haven't done it here, since it's a lot of code to update, but it could be a good idea for new test code.

I've run the build a couple of times on a Linux VM, and I'm pretty much only seeing test failures in the JPA server module, and those failures don't seem related to which port the server is running on. I'll raise separate issue to track fixing the failures I saw.

@srdo srdo force-pushed the srdo:issue-1325 branch from 585b77d to fa3dd20 Jun 10, 2019

Issue-1325: Let OS assign random ports to servers during testing, and…
… let tests get the port of the running server once the OS has assigned a port. Delete PortUtil and similar utilities.
@srdo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

The JettyUtil class in hapi-fhir-base requires that I add jetty as a dependency. If there's a better module for the utility, I'm happy to move it.

@srdo srdo force-pushed the srdo:issue-1325 branch from fa3dd20 to a69a498 Jun 10, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jun 10, 2019

Coverage Status

Coverage decreased (-0.02%) to 75.835% when pulling a69a498 on srdo:issue-1325 into dd203ae on jamesagnew:master.

@jamesagnew

This comment has been minimized.

Copy link
Owner

commented Jun 10, 2019

Super cool. Definitely want to review this with a bit more detail a bit later, but one comment:

Some tests were closing servers in @afterclass methods, while also clearing all static fields in a different @afterclass, using the TestUtil.clearAllStaticFieldsForUnitTest() method

That clearAllStaticFieldsForUnitTest method may well not be needed any more truthfully. It was added a long time ago, when the FhirContext consumed quite a bit of memory and the tests were running out. It probably makes sense to just remove it at this point...

@fil512

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@srdo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

I'm happy to try removing clearAllStaticFieldsForUnitTest, but as it is used in ~500 files, I'd rather do that in another PR.

@jamesagnew
Copy link
Owner

left a comment

This is just awesome. I made one comment, regarding adding a test module so that we can avoid adding any new non-test deps to the hapi-fhir-base module. I'm happy to help with that if needed.

Thanks, this seems like a huge improvement!

<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-server</artifactId>
</dependency>

This comment has been minimized.

Copy link
@jamesagnew

jamesagnew Jun 13, 2019

Owner

I think we should avoid introducing this dependency- Given that all kinds of things depend on hapi-fhir-base, including Android, OSGI, client, etc., having a server dependency in the core JAR is likely to cause unintended issues.

My vote would be to introduce a new module called hapi-fhir-test-utils that hosts JettyTest that the tests depend on.

This comment has been minimized.

Copy link
@srdo

srdo Jun 13, 2019

Author Contributor

Makes sense. This change is in the latest commit for ease of review.

@srdo srdo force-pushed the srdo:issue-1325 branch from 3fad6c3 to e5ab804 Jun 14, 2019

@srdo srdo force-pushed the srdo:issue-1325 branch from e5ab804 to a711bda Jun 14, 2019

@jamesagnew jamesagnew merged commit b56f595 into jamesagnew:master Jun 14, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

jamesagnew added a commit that referenced this pull request Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.