Remove all test that use Dancer2::Test (except route PODs): #552

Merged
merged 1 commit into from Apr 2, 2014

Projects

None yet

5 participants

@xsawyerx
Member
xsawyerx commented Apr 1, 2014

Finally.

Dancer2::Test is 864 lines of code, including documentation. It tries to fake
whatever is necessary to run a request against the Dancer2 code.

It is long, exhaustive, and hairy. It has subtle edge cases that required work
on both D1 and D2. It has been the bane of much testing code in Dancer. It also
has at least one bug (GH #544).

More than all of these, it simply isn't required. At least not the way it works
now. Plack::Test covers everything it does, except for checking POD coverage of
routes - which could be kept.

For now I've changed all the code that uses it to use Plack::Test instead. The
code is clearer and more maintainable.

Dancer2::Test will move to a relaxed deprecation cycle. We will add
documentation that shows a more preferred approach, including how to rewrite
existing tests.

An additional process requires having a simple method for fetching the PSGI app,
that will also call the "finish" methods on each Dancer application - tasks
that are more verbose without Dancer2::Test and expose more of the backend than
should be exposed (because it might actually change).

@xsawyerx xsawyerx Remove all test that use Dancer2::Test (except route PODs):
Finally.

Dancer2::Test is 864 lines of code, including documentation. It tries to fake
whatever is necessary to run a request against the Dancer2 code.

It is long, exhaustive, and hairy. It has subtle edge cases that required work
on both D1 and D2. It has been the bane of much testing code in Dancer. It also
has at least one bug (GH #544).

More than all of these, it simply isn't required. At least not the way it works
now. Plack::Test covers everything it does, except for checking POD coverage of
routes - which could be kept.

For now I've changed all the code that uses it to use Plack::Test instead. The
code is clearer and more maintainable.

Dancer2::Test will move to a relaxed deprecation cycle. We will add
documentation that shows a more preferred approach, including how to rewrite
existing tests.

An additional process requires having a simple method for fetching the PSGI app,
that will also call the "finish" methods on each Dancer application - tasks
that are more verbose without Dancer2::Test and expose more of the backend than
should be exposed (because it might actually change).
4ca4529
@racke
Member
racke commented Apr 1, 2014

Yes, please merge this precious patch!

@shumphrey
Collaborator

👍

@veryrusty
Member

👍 @xsawyerx++ (and @racke++ too)

While I have a preference for using variations of the following to get the psgi app coderef instead of reaching into the internals (and having to deal with calling finish), it's not something that should hold back this important work being merged!

BEGIN {$ENV{DANCER_APPHANDLER} = 'PSGI';} 
use Dancer2;
my $app = dance();

(the BEGIN block is just as ugly!)

@xsawyerx
Member
xsawyerx commented Apr 2, 2014

@veryrusty The last paragraph in the PR message talks about it.

I thought of two things we can do:

  1. Keep Dancer2::Test for stuff like route POD coverage and getting the apps.
  2. Add a DSL keyword like psgi_app to get the app (calling the ->finish methods and all)

I would opt for 2. What do you think?

@shumphrey
Collaborator

Providing a convenient psgi_app dsl gives people the flexibility to do testing however they want.

@drebolo
drebolo commented Apr 2, 2014

I can create a Dancer2::Test::RoutePOD ( as suggested by shumphrey ) or something along that line.

@racke
Member
racke commented Apr 2, 2014

On 04/02/2014 02:12 PM, Sawyer X wrote:

@veryrusty The last paragraph in the PR message talks about it.

I thought of two things we can do:

  1. Keep Dancer2::Test for stuff like route POD coverage and getting the apps.
  2. Add a DSL keyword like psgi_app to get the app (calling the ->finish methods and all)

I would opt for 2. What do you think?

+1 to 2. for sure. Dancer2::Test is one of the black sheep bumping on the dance floor.

Regards
Racke

Perl and Dancer Development

Visit our Open Source conference on E-commerce:

http://www.ecommerce-innovation.com/

@xsawyerx
Member
xsawyerx commented Apr 2, 2014

I don't know if having it in a different namespace makes much difference.

We're slowly phasing Dancer2::Test (meaning warning and keeping it in core for a while). We can keep it for things that aren't covered by Plack::Test like the route POD testing.

This means that with time, when the phasing out is complete (we don't really care how long it's still in core and warns), Dancer2::Test will only include the route POD tests anyway.

@drebolo What would be nice, I think, would be to go over the route POD testing code and making sure it's sane, and doesn't use the other stuff in Dancer2::Test which we're gonna deprecate.

@racke
Member
racke commented Apr 2, 2014

Going to hit the shelves now, thanks xsawyerx and everyone else involved!

@racke racke merged commit 66db1ee into master Apr 2, 2014

1 check passed

Details default The Travis CI build passed
@drebolo
drebolo commented Apr 2, 2014

@xsawyerx after a quick look, I think pod_coverage is not using any Dancer2 :: Test code and it seems to me sane enough.

@xsawyerx
Member
xsawyerx commented Apr 2, 2014

@drebolo Thanks for checking. I think we could keep those in Dancer2::Test for now then.

@xsawyerx xsawyerx deleted the feature/psgi-tests branch Apr 3, 2014
@veryrusty veryrusty added a commit that referenced this pull request Apr 25, 2014
@veryrusty veryrusty Add psgi_app DSL keyword to return psgi coderef for current app.
From a suggestion in #552; add a new DSL keyword that returns the
psgi coderef for the app (and only THE app if there is more than one)
8b606be
@veryrusty veryrusty added a commit that referenced this pull request Jul 24, 2014
@veryrusty veryrusty Add psgi_app DSL keyword to return psgi coderef for current app.
From a suggestion in #552; add a new DSL keyword that returns the
psgi coderef for the app (and only THE app if there is more than one)
a219b3c
@xsawyerx
Member

PSGI app DSL keyword added by recent merge. Fantastic work, everyone.

@JRaspass JRaspass added a commit to JRaspass/Dancer2 that referenced this pull request Jul 24, 2014
@veryrusty veryrusty Add psgi_app DSL keyword to return psgi coderef for current app.
From a suggestion in #552; add a new DSL keyword that returns the
psgi coderef for the app (and only THE app if there is more than one)
534e3bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment