Skip to content
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

[Laravel 6] Add support for Laravel Dusk tests #4919

Merged
merged 26 commits into from
Feb 3, 2020

Conversation

bennothommo
Copy link
Contributor

Forms part of work done in #4893. Replaces #3051.

This implements support for Laravel Dusk, a more friendly way for doing comprehensive browser-based testing.

This PR significantly refactors the testing base to allow these tests to be included for both core, as well as a part of plugins.

Ben Thomson added 5 commits January 29, 2020 13:57
Replaces the original Selenium browser testing.

Refs: https://laravel.com/docs/6.x/dusk
The BrowserTestCase and PluginTestCase classes are intended to be available to plugins. This change uses traits to share common code between them.
Copy link
Contributor

@LukeTowers LukeTowers left a comment

Choose a reason for hiding this comment

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

LGTM! Still need a PR to the docs documenting all this stuff now and an update the L6 upgrade guide

@LukeTowers
Copy link
Contributor

@bennothommo

If you are manually registering Dusk's service provider, you should never register it in your production environment, as doing so could lead to arbitrary users being able to authenticate with your application.

How can we protect users from this?

@bennothommo
Copy link
Contributor Author

@LukeTowers There's a couple of things in play already to prevent this from happening:

  • We're using automatic registration of the service provider, via using autoload-dev in composer.json.
  • The tests run in a different environment to the live site by replacing .env with .env.dusk on the fly, as well as the configuration in config/dusk/. The database runs in SQLite for the tests by default, and requires manual intervention in order to use any other database.

It could be possible that with that message, they're referring to the Dusk routes they provide in their service provider, which can basically let you log in as any user. They don't actually work with October, so we could potentially extend the Service Provider and just remove those routes.

@LukeTowers
Copy link
Contributor

@bennothommo I would feel most comfortable if we removed those routes entirely.

@bennothommo
Copy link
Contributor Author

@LukeTowers instead of extending the service provider, I've just disabled auto-discovery for Laravel Dusk, and am using the system module's service provider to load the console commands as necessary. Those routes should no longer be loaded.

@bennothommo
Copy link
Contributor Author

This is ready to review and merge into the main L6 branch. I'll add the tests/README.md changes to the October docs once reviewed.

@LukeTowers
Copy link
Contributor

LGTM!

@LukeTowers
Copy link
Contributor

@bennothommo actually have we dealt with re: unit tests related to rainlab/translate-plugin#509 & rainlab/translate-plugin#546
DatabaseServiceProvider does the following when registering:

Model::clearBootedModels();
Model::clearExtendedClasses();
Model::flushDuplicateCache();

Should we also add Model::flushEventListeners(); (used in https://github.com/octobercms/october/blob/master/tests/unit/cms/classes/CmsCompoundObjectTest.php for Halcyon, but also applicable to Eloquent) and then also implement a Model::flushDuplicateCache() in Halcyon models and add those four lines to HalcyonServiceProvider?

@bennothommo bennothommo merged commit 65c3a88 into wip/laravel-6 Feb 3, 2020
@bennothommo bennothommo deleted the wip/dusk/laravel-6 branch February 3, 2020 04:21
bennothommo pushed a commit that referenced this pull request Feb 7, 2020
The Browser tests will be made into a RainLab plugin. (https://github.com/rainlab/dusk-plugin)
@bennothommo bennothommo removed this from the In Progress milestone Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants