-
Notifications
You must be signed in to change notification settings - Fork 29
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
E2E tests setup with laravel dusk #234
Conversation
Wow, this looks great! Anything you want my input on? |
@brendt Thanks. Yes feel free to suggest improvements to what I have so far. I am open to solutions on how to solve the selenium docker image issue, not sure if this PR should be blocked by this issue though. What I would like to do in this PR is the following:
|
Can we add an Blade::directive('dusk', function ($expression) {
if ( ! app()->isProduction()) {
return "<?php echo 'dusk=\"' . $expression . '\"'; ?>";
}
return null;
}); Then in the blade files |
@joshbonnick Honestly I'm ok with any suggestion, the way I started to add tests was to use this page objects which also allows you to map some css selectors to some dusk specific selector. Honestly I'm not sure what would be the best way to handle this. But I can say I didn't think of adding a custom blade directive. I have to admin that it is the first time writing laravel dusk tests, so not really that up to date with best practices and popular patterns. |
version: '3'
services:
selenium:
image: ${SELENIUM_IMAGE}
extra_hosts:
- 'host.docker.internal:host-gateway'
volumes:
- '/dev/shm:/dev/shm'
networks:
- sail In the dusk env file
You should also add
|
@joshbonnick I tried to add this, but I don't think this is valid in blade, I mean you can't use the directive in the way you suggested, maybe I'm just not familiar with this. In any case I not sure if we should bother that much with removing the dusk selector in prod. @brendt do you have any opinions/ideas/suggestions on this? |
Thanks, I added this, it is working, but just one thing, it needs to be added to .env file, I think sail looks at .env when you run the commands like
I added this, indeed I saw some small improvements in test run time, thanks for suggesting it. |
|
||
namespace PHPSTORM_META { | ||
|
||
override(\Laravel\Dusk\Browser::visit(0), type(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird trick to improve a bit the developer experience with page methods.
Basically with this PhpStorm doesn't complain about undefined method, when doing visit(new SomePage)
followed by some methods define in SomePage, here is an example from LoginPageTest:
@brendt please let me know how you feel about this, it is a bit unconventional and I wasn't able to find a better way a more general way of improving the page methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally fine with it. I use the Laravel Idea plugin, which I think supports this out of the box, but I'm fine adding it via phpstorm meta as well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use Laravel Idea plugin as well, but I wasn't able to make it work only with that.
…ory state method.
selenium container
cace390
to
b092c36
Compare
I think it is better now thanks, need to be a bit more careful in the future with fork syncing |
@brendt Do I need to do anything special to get Qodana to pass? :) |
<div | ||
class="container flex justify-between text-white gap-4 items-center m-auto relative px-2" | ||
x-data="{ open: false }" | ||
> | ||
<div class="dark:text-gray-200 text-lg md:text-xl font-bold relative z-20"> | ||
<div dusk="header-logo" class="dark:text-gray-200 text-lg md:text-xl font-bold relative z-20"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use @dusk('header-logo)
. I kind of thought Laravel had this built-in, but apparently it hasn't? 🤔
The idea would be that @dusk
will only write the x-dusk
attribute when not in production, so that we don't have additional dusk attributes on the real site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'll try to look more into this, I haven't found anything here https://laravel.com/docs/10.x/dusk#dusk-selectors
Maybe we should just do it via a function? function dusk(string $name): ?HtmlString
{
if (app()->isProduction()) {
return null;
}
return new HtmlString($name);
} <div {{ dusk('header-logo') }} (I didn't test this, it could have syntax errors) |
Apart from the dusk attribute, this looks all super nice! I'm happy to merge it when the attribute thing has been fixed. |
use Illuminate\Support\HtmlString; | ||
|
||
if (! function_exists('dusk')) { | ||
function dusk(string $selector): ?HtmlString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendt I tried with this helper function, it works on html elements but not on blade components :(.
This is the error I get when I attempt to use this helper on a blade component.
<x-home.title {{dusk('test')}}>
{{ __('Open RFCs') }}
</x-home.title>
Not really sure how else to approach this at this point :), I'm willing to work more on this but I would need some ideas/suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think the dusk function should be move to the component itself, no? Otherwise you'd have to repeat it every time you add the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but we need to keep this limitation in mind, if that's ok, I can try to use it and see how it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's ok :)
# Conflicts: # resources/views/components/card-link.blade.php # resources/views/layouts/base.blade.php
# Conflicts: # resources/views/components/footer.blade.php
@brendt when you have time please have another look |
Awesome! |
This PR does the following:
Closes #231