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

Generalize DropwizardAppRule for other test frameworks #488

Merged
merged 2 commits into from Jan 4, 2015

Conversation

Projects
None yet
7 participants
@csabapalfi
Contributor

csabapalfi commented Mar 7, 2014

Hi,

I was looking at starting/stopping a Dropwizard app in Cucumber tests and DropwizardAppRule has all the functionality I need but obviously it doesn't expose startIfRequired and stop methods.

I'd happy to extract a DropWizardAppTestSupport class from DropwizardAppRule and make the DropwizardAppRule depend on that and contribute the code if there's any interest.

What do you think?

Thanks,
Csaba

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Mar 7, 2014

Alternatively we can just make startIfRequired and a new stop method public on DropwizardAppRule but I would prefer a separate test support class and the rule using that.

@nicktelford

This comment has been minimized.

Contributor

nicktelford commented Mar 7, 2014

I like this idea quite a bit. I'm working on a Scala module that will need support for idiomatic testing at some point, so it'd be nice to be able to hook Specs2/ScalaTest in to the same. We may even want to extend the same idea to the other JUnit TestRules that are springing up for the same reason. Better support for testing is something I'd really like to develop in the future so that Dropwizard makes it easy not only to build applications, but to build out the tests for them too.

One thing though: because this would be a new feature, it is highly unlikely to be merged for 0.7.0 (though I'd be happy to accept it for the next release).

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Mar 7, 2014

Added some code which extracts DropwizardTestSupport from DropwizardAppRule while keeping complete backwards compatibility.

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Jul 14, 2014

Hey, are you looking at merging this anytime soon?

@hslater-okta

This comment has been minimized.

hslater-okta commented Aug 4, 2014

Is there still a plan to merge this?

@ddunwoody

This comment has been minimized.

ddunwoody commented Sep 2, 2014

I've been using this in conjuction with cucumber-guice with great success to more explicitly control server startup/shutdown, so I'd +1 this for merge, please.

@joschi

This comment has been minimized.

Member

joschi commented Nov 9, 2014

@csabapalfi Would you mind rebasing this PR on the current master?

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Nov 10, 2014

I actually no longer use DropWizard, but will try to have a look this week.

@joschi

This comment has been minimized.

Member

joschi commented Jan 4, 2015

If someone steps up to update the PR to apply cleanly to current master, we'll happily merge it. (@ddunwoody, @hslater-okta)

@ddunwoody

This comment has been minimized.

ddunwoody commented Jan 4, 2015

I'll take a look this week. It's my last week on the project that uses this, so it would be a nice feeling to get it merged in.

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Jan 4, 2015

Sorry about the delay.

Re-done the change on the latest master (as it diverged a lot).

2 tests were failing locally when run with all others (but passing in isolation). That still needs fixing.

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Jan 4, 2015

The patch keeps everything backwards compatible. Is that required? If not then the code can be a bit simpler in some cases.

@coveralls

This comment has been minimized.

coveralls commented Jan 4, 2015

Coverage Status

Changes Unknown when pulling ed419bf on csabapalfi:test-support into * on dropwizard:master*.

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Jan 4, 2015

Found the cause of the test failures. One of the test was not cleaning up after itself properly and left an extra system property confusing the tests run afterwards.

@coveralls

This comment has been minimized.

coveralls commented Jan 4, 2015

Coverage Status

Changes Unknown when pulling 09d9b54 on csabapalfi:test-support into * on dropwizard:master*.

@csabapalfi

This comment has been minimized.

Contributor

csabapalfi commented Jan 4, 2015

Just spotted that the same test problem is already on master. Introduced by 9848d9b

If you merge my PR and that'll fix the tests on master, too.

@joschi

This comment has been minimized.

Member

joschi commented Jan 4, 2015

Great! Thanks for the PR.

joschi added a commit that referenced this pull request Jan 4, 2015

Merge pull request #488 from csabapalfi/test-support
Generalize DropwizardAppRule for other test frameworks

@joschi joschi merged commit 991b1c7 into dropwizard:master Jan 4, 2015

@joschi joschi added this to the 0.8.0 milestone Jan 4, 2015

@joschi joschi removed the needs code label Jan 4, 2015

@ddunwoody

This comment has been minimized.

ddunwoody commented Jan 4, 2015

Thanks everyone. I was saving this for work in the morning, but looks like you beat me to it!

@gcabero

This comment has been minimized.

gcabero commented on ed419bf Apr 26, 2017

This change has broken our dropwizard services that don't have default constructors (use of Tapestry IOC for injecting services). The comment about overriding newApplication() method for classes that extend from DropwizardAppRule doesn't seem to work as when it tries to start the server on the before() method will call the DropWizardTestSupport (composition). I guess the idea is to override the constructor that sets DropwizardTestSupport

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