Testing Mojo::DOM with Test::Mojo #1046

marcusramberg · 2017-01-30T22:05:36Z

Summary

Expose dom as an attribute on Test::Mojo to allow testing Mojo::DOM objects without involving a user agent. Revised version of #1045 with new tests instead of changing t/mojo/dom.t

my $t=Test::Mojo->new();
$t->dom(Mojo::DOM->new('<foo>bar</foo>'));
$t->is_text('foo','bar');

Motivation

This makes it easy to use Test::Mojo to test the contents of HTML/XML strings.

kraih · 2017-01-30T22:15:19Z

t/mojolicious/lib/TestApp.pm

@@ -0,0 +1,10 @@
+package TestApp;


This class doesn't really add much value, does it? Couldn't you just as well have used Mojolicious::Lite in the test file?

kraih · 2017-01-30T22:18:12Z

Looks like your pull request is borked, it contains my last commit.

marcusramberg · 2017-01-30T22:29:21Z

Fixed the extra commit, and moved the test app into the t/test-mojo.t file - Will rebase and force push if the pull request is accepted.

kraih · 2017-01-30T22:37:12Z

Move it into t/mojo/test.t.

kraih · 2017-01-30T22:38:32Z

Changes

@@ -1,5 +1,6 @@

 7.24  2017-01-30
+  - Expose dom as an attribute on Test::Mojo.


No need for a Changes entry, i'll rewrite it anyway. 😉

kraih · 2017-01-30T22:38:54Z

lib/Test/Mojo.pm

@@ -17,7 +17,8 @@ use Mojo::Util qw(decode encode);
 use Test::More ();

 has [qw(message success tx)];
-has ua => sub { Mojo::UserAgent->new->ioloop(Mojo::IOLoop->singleton) };
+has ua  => sub { Mojo::UserAgent->new->ioloop(Mojo::IOLoop->singleton) };
+has dom => sub { shift->tx->res->dom };


Order is not alphabetical.

kraih · 2017-01-30T22:40:51Z

lib/Test/Mojo.pm

+=head2 dom
+
+  my $dom = $t->dom;
+  $t     = $t->dom(Mojo::DOM->new);


Indentation is borked here.

kraih · 2017-01-30T22:45:35Z

t/test-mojo.t

+use Mojo::Base -strict;
+
+BEGIN {
+  $ENV{MOJO_MODE}    = 'testing';


The mode does not appear to be used anywhere in the test.

kraih · 2017-01-30T22:46:06Z

t/test-mojo.t

+  $ENV{MOJO_MODE}    = 'testing';
+  $ENV{MOJO_REACTOR} = 'Mojo::Reactor::Poll';
+}
+use FindBin;


FindBin does not appear to used anywhere.

…ctly with Test::Mojo

jberger

As I mentioned on the previous PR, I have wanted something like this before. This is a fairly clean and consistent way to achieve it (consistent with the tx attribute).

nicomen · 2017-01-31T23:39:32Z

Long sought-after and clean enough. Good work ;)

kraih · 2017-02-01T18:02:30Z

I'd really like to hear more about how you're planning to use this.

jberger · 2017-02-01T18:34:48Z

My use has usually been with mixed type apis. So if there's say a json api that has a key who's value is HTML. This happens rather commonly with backend-rendered content for a single page front-end.

$t->get('/some/resource/with/html.json')
  ... # json tests

$t->dom(Mojo::DOM->new($t->tx->res->json('/contains/html')));
$t->... # html tests

at least, that's how I've wanted it in the past.

kraih

I'm neutral on this feature.

kraih · 2017-02-01T23:04:42Z

@jhthorsen, @amenonsen and @tempire still have to review.

jhthorsen · 2017-02-02T03:52:53Z

I'm neutral. There are times when I want to poke into the "res" for manual testing or debugging, but I don't think the "dom" attribute is special for my cases.

kraih · 2017-02-02T11:59:26Z

@jhthorsen Please do a formal review, so we can see the result "Reviewers" list at the top.

jhthorsen

I'm not sure if I'm a fan of having two attributes holding the Mojo::DOM object and require synchronizing. Seems to me that this functionality should be achieved in a different class, with a clear statement for what functionality it provides. But I would rather solve the problem with a simple Lite app in the test instead of requesting a change in Test::Mojo.

kraih · 2017-02-03T23:31:08Z

Guess i'm slightly 👎 on this as well, something just doesn't feel right about it.

kraih · 2017-02-05T19:50:24Z

I'm afraid this pull request did not pass the vote as it ended in a tie, which means i had to make the final decision. What ultimately made me decide against it was the fact that the feature can be easily implemented as a Test::Mojo::Role and that there was only a single rather uncommon use case presented.

kraih reviewed Jan 30, 2017

View reviewed changes

lib/Test/Mojo.pm Outdated

=head2 dom

my $dom = $t->dom;

$t = $t->dom(Mojo::DOM->new);

Copy link

Member

kraih Jan 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is borked here.

kraih reviewed Jan 30, 2017

View reviewed changes

marcusramberg requested review from jhthorsen and jberger January 31, 2017 20:57

Expose dom as an attribute to allow testing Mojo::DOM structures dire…

54940ee

…ctly with Test::Mojo

jberger approved these changes Jan 31, 2017

View reviewed changes

kraih requested review from tempire, amenonsen, kraih and jberger and removed request for jberger February 1, 2017 22:12

kraih reviewed Feb 1, 2017

View reviewed changes

jhthorsen reviewed Feb 2, 2017

View reviewed changes

kraih removed request for tempire and amenonsen February 3, 2017 23:30

kraih closed this Feb 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing Mojo::DOM with Test::Mojo #1046

Testing Mojo::DOM with Test::Mojo #1046

marcusramberg commented Jan 30, 2017

kraih Jan 30, 2017

kraih commented Jan 30, 2017

marcusramberg commented Jan 30, 2017

kraih commented Jan 30, 2017

kraih Jan 30, 2017

kraih Jan 30, 2017

kraih Jan 30, 2017

kraih Jan 30, 2017

kraih Jan 30, 2017

jberger left a comment

nicomen commented Jan 31, 2017

kraih commented Feb 1, 2017

jberger commented Feb 1, 2017

kraih left a comment

kraih commented Feb 1, 2017

jhthorsen commented Feb 2, 2017

kraih commented Feb 2, 2017

jhthorsen left a comment

kraih commented Feb 3, 2017

kraih commented Feb 5, 2017 •

edited

Loading

		@@ -1,5 +1,6 @@

		7.24 2017-01-30
		- Expose dom as an attribute on Test::Mojo.

Testing Mojo::DOM with Test::Mojo #1046

Testing Mojo::DOM with Test::Mojo #1046

Conversation

marcusramberg commented Jan 30, 2017

Summary

Motivation

kraih Jan 30, 2017

Choose a reason for hiding this comment

kraih commented Jan 30, 2017

marcusramberg commented Jan 30, 2017

kraih commented Jan 30, 2017

kraih Jan 30, 2017

Choose a reason for hiding this comment

kraih Jan 30, 2017

Choose a reason for hiding this comment

kraih Jan 30, 2017

Choose a reason for hiding this comment

kraih Jan 30, 2017

Choose a reason for hiding this comment

kraih Jan 30, 2017

Choose a reason for hiding this comment

jberger left a comment

Choose a reason for hiding this comment

nicomen commented Jan 31, 2017

kraih commented Feb 1, 2017

jberger commented Feb 1, 2017

kraih left a comment

Choose a reason for hiding this comment

kraih commented Feb 1, 2017

jhthorsen commented Feb 2, 2017

kraih commented Feb 2, 2017

jhthorsen left a comment

Choose a reason for hiding this comment

kraih commented Feb 3, 2017

kraih commented Feb 5, 2017 • edited Loading

kraih commented Feb 5, 2017 •

edited

Loading