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

Add data-dusk DOM hooks (for less brittle tests) #343

Merged
merged 2 commits into from Aug 28, 2017

Conversation

Projects
None yet
6 participants
@calebporzio
Contributor

calebporzio commented Aug 24, 2017

TL;DR;

// before
<div class="post-panel m-b-lg"><p>Some Post Title</p></div>
$browser->click('.post-panel.m-b-lg p');

// after
<div class="post-panel m-b-lg"><p data-dusk="post-title">Some Post Title</p></div>
$browser->click('@post-title');

Depending on prone-to-change ui classes in Dusk tests like div div div.campaign-panel--large ul li can make for really brittle acceptance tests.

There is a slightly better solution from the jquery days: .js-campaign-title

Something like .dusk-campaign-title would do the trick, but I personally don't like muddying up my class lists.

I've come up with a solution that I think is pretty BA: <li class="active" data-dusk="campaign-title">...

My selectors in dusk now look like: $browser->click('[data-dusk="campaign-title"]') - which works but makes me a little sad. I've used macros like $browser->hook('campaign-title') in the past, but things like that always feel hidden to me.

I propose leveraging the existing '@campaign-title' => 'div div.campaign-panel...' idiom in Dusk and creating a fallback for data-dusk

Now, anywhere in your DOM you can add data-dusk="something", and it is immediately available via $browser->click('@something')

It's ballsy, I know - but people seem to really dig it.

@mattstauffer

This comment has been minimized.

Show comment
Hide comment
@mattstauffer

mattstauffer Aug 24, 2017

I don't have it in front of me but I think modern browsers may not need the data- prefix, which means you could simplify this to duskhook="title" or something simpler. Not at my computer but worth checking out. Love this idea though!

mattstauffer commented Aug 24, 2017

I don't have it in front of me but I think modern browsers may not need the data- prefix, which means you could simplify this to duskhook="title" or something simpler. Not at my computer but worth checking out. Love this idea though!

@calebporzio

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio Aug 24, 2017

Contributor

Interesting @mattstauffer - that scares me a little - but less if you wrap these data-dusk hooks in something that hides them from production (I think that was Adam's suggestion a while back) - like a blade directive:

<li class="campaign__name" @dusk('campaign-name')></li>
Contributor

calebporzio commented Aug 24, 2017

Interesting @mattstauffer - that scares me a little - but less if you wrap these data-dusk hooks in something that hides them from production (I think that was Adam's suggestion a while back) - like a blade directive:

<li class="campaign__name" @dusk('campaign-name')></li>
@jakebathman

This comment has been minimized.

Show comment
Hide comment
@jakebathman

jakebathman Aug 24, 2017

I like the @dusk() idea, and generally think cluttering up the class space with these non-fragile test classes is crummy. This would be great to get that out of the production DOM. 👍

jakebathman commented Aug 24, 2017

I like the @dusk() idea, and generally think cluttering up the class space with these non-fragile test classes is crummy. This would be great to get that out of the production DOM. 👍

@calebporzio

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio Aug 24, 2017

Contributor

@jakebathman - thanks for the input Jake. I agree - if this goes through, I'll PR a blade directive, or at least tweet a macro or something.

Contributor

calebporzio commented Aug 24, 2017

@jakebathman - thanks for the input Jake. I agree - if this goes through, I'll PR a blade directive, or at least tweet a macro or something.

@jakebathman

This comment has been minimized.

Show comment
Hide comment
@jakebathman

jakebathman Aug 24, 2017

Core could definitely use a pre-defined directive to close the loop on this kind of testing pattern. Good stuff.

jakebathman commented Aug 24, 2017

Core could definitely use a pre-defined directive to close the loop on this kind of testing pattern. Good stuff.

@mattstauffer

This comment has been minimized.

Show comment
Hide comment
@mattstauffer

mattstauffer Aug 24, 2017

@calebporzio Curious: What's scaring about dropping the data- prefix? It's basically like Angular or Vue directives.

mattstauffer commented Aug 24, 2017

@calebporzio Curious: What's scaring about dropping the data- prefix? It's basically like Angular or Vue directives.

@calebporzio

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio Aug 24, 2017

Contributor

@mattstauffer - good call - maybe I like data-dusk because people are more familiar with what data attributes are used for? Idk

Contributor

calebporzio commented Aug 24, 2017

@mattstauffer - good call - maybe I like data-dusk because people are more familiar with what data attributes are used for? Idk

@damiani

This comment has been minimized.

Show comment
Hide comment
@damiani

damiani Aug 24, 2017

Contributor

Particularly if coupled with an optional @dusk Blade directive that hides the value in production, I like @mattstauffer's idea of ditching the data- prefix, but it seems cleaner to just calling it dusk rather than duskhook so there's a parallel between both implementations.

So you'd either have:

<li class="campaign__name" @dusk('campaign-name')></li>

or

<li class="campaign__name" dusk="campaign-name"></li>

both of which could be referenced by

$browser->click('@campaign-name')
Contributor

damiani commented Aug 24, 2017

Particularly if coupled with an optional @dusk Blade directive that hides the value in production, I like @mattstauffer's idea of ditching the data- prefix, but it seems cleaner to just calling it dusk rather than duskhook so there's a parallel between both implementations.

So you'd either have:

<li class="campaign__name" @dusk('campaign-name')></li>

or

<li class="campaign__name" dusk="campaign-name"></li>

both of which could be referenced by

$browser->click('@campaign-name')
@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Aug 24, 2017

I agree with Keith. @dusk('campaign-name') speaks my language.

DanielCoulbourne commented Aug 24, 2017

I agree with Keith. @dusk('campaign-name') speaks my language.

@calebporzio

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio Aug 24, 2017

Contributor

I like that: dusk="campaign-name" and @dusk="campaign-name"

Contributor

calebporzio commented Aug 24, 2017

I like that: dusk="campaign-name" and @dusk="campaign-name"

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Aug 24, 2017

Member

I like it combined with the Blade stuff.

Member

taylorotwell commented Aug 24, 2017

I like it combined with the Blade stuff.

@calebporzio

This comment has been minimized.

Show comment
Hide comment
@calebporzio

calebporzio Aug 25, 2017

Contributor

@taylorotwell - I changed the implementation to look for dusk="something" instead of data-dusk="something"

However, I couldn't think of a way to add a blade directive seeing as how the DuskServiceProvider typically isn't loaded in production. The only options I could think of:

  • separate service provider
  • register the directive in laravel/framework seeing as the directive doesn't actually depend on having laravel/dusk installed
  • Have users always register DuskServiceProvider in 2.0 and conditionally load the "unsafe for production" routes inside the service provider.

The best option IMO is just to ship it with framework 5.5 - I'm interested to hear your thought though, hoping you'll say: "why don't you just..." and I'll go "aha!"

  • Thanks
Contributor

calebporzio commented Aug 25, 2017

@taylorotwell - I changed the implementation to look for dusk="something" instead of data-dusk="something"

However, I couldn't think of a way to add a blade directive seeing as how the DuskServiceProvider typically isn't loaded in production. The only options I could think of:

  • separate service provider
  • register the directive in laravel/framework seeing as the directive doesn't actually depend on having laravel/dusk installed
  • Have users always register DuskServiceProvider in 2.0 and conditionally load the "unsafe for production" routes inside the service provider.

The best option IMO is just to ship it with framework 5.5 - I'm interested to hear your thought though, hoping you'll say: "why don't you just..." and I'll go "aha!"

  • Thanks
@jakebathman

This comment has been minimized.

Show comment
Hide comment
@jakebathman

jakebathman Aug 25, 2017

I had always assumed it’d be part of laravel/framework and just render the dusk=“foo” attributes. Maybe I wasn’t thinking too deeply on it, though?

jakebathman commented Aug 25, 2017

I had always assumed it’d be part of laravel/framework and just render the dusk=“foo” attributes. Maybe I wasn’t thinking too deeply on it, though?

@taylorotwell taylorotwell merged commit f4870cf into laravel:master Aug 28, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment