- Issue created by @Spokje
- Merge request !6095Enable modules through Nightwatch API when not testing module enabling β (Closed) created by Spokje
- Issue was unassigned.
- Status changed to Needs review
8 months ago 7:21am 10 January 2024 - Status changed to RTBC
8 months ago 4:27pm 10 January 2024 - πΊπΈUnited States smustgrave
Change seems good.
Searched for .drupalRelativeURL('/admin/modules') and saw the 2 instances were updated.
Hopefully this helps move nightwatch tests to be more useable.
- Status changed to Fixed
8 months ago 6:31pm 12 January 2024 - π¬π§United Kingdom longwave UK
Good idea, we have the API so let's use it! Backported to 10.2.x to keep tests in sync.
Committed and pushed a2f51b88d5 to 11.x and 977e680afb to 10.2.x. Thanks!
-
longwave β
committed 977e680a on 10.2.x
Issue #3413665 by Spokje: Enable modules through Nightwatch API when not...
-
longwave β
committed 977e680a on 10.2.x
-
longwave β
committed a2f51b88 on 11.x
Issue #3413665 by Spokje: Enable modules through Nightwatch API when not...
-
longwave β
committed a2f51b88 on 11.x
-
alexpott β
committed 3d2f1509 on 10.2.x
Revert "Issue #3413665 by Spokje: Enable modules through Nightwatch API...
-
alexpott β
committed 3d2f1509 on 10.2.x
-
alexpott β
committed 34258c08 on 11.x
Revert "Issue #3413665 by Spokje: Enable modules through Nightwatch API...
-
alexpott β
committed 34258c08 on 11.x
- Status changed to Needs work
8 months ago 6:17pm 18 January 2024 - π¬π§United Kingdom alexpott πͺπΊπ
andypost β credited alexpott β .
- π¬π§United Kingdom alexpott πͺπΊπ
Unfortunately this resulted in a flood of Nightwatch errors in modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5CodeSyntaxTest.js - I could not get this to pass locally without reverting this.
I suspect this is due to async and perform doing a retry and things getting very broken.
- π¬π§United Kingdom longwave UK
longwave β changed the visibility of the branch 3413665-enable-modules-through to active.
- Merge request !6231Issue #3413665 by Spokje: Enable modules through Nightwatch... β (Open) created by longwave
- Status changed to Needs review
8 months ago 6:36pm 18 January 2024 - π¬π§United Kingdom longwave UK
Modified
drupalInstallModule()
so it can install more than one module at a time. - π¬π§United Kingdom longwave UK
I don't even think the test needs Field UI, we could just remove that, but we should probably fix the multiple-install issue while we are here?
- π¬π§United Kingdom alexpott πͺπΊπ
Yeah swapping the order of the modules being installed in
before(browser) {
completely breaks the modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5CodeSyntaxTest.js locally.Also what is super fun is that core/modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5EditorHeightTest.js does not fail :) It tries to enable 2 modules⦠and again because the test does not actually need field ui it is fine. But
// Set fixed (desktop-ish) size to ensure a maximum viewport. browser.resizeWindow(1920, 1080);
somehow makes it pass.
We need to do here is add a test for enabling multiple modules via something like
browser .drupalInstall({ installProfile: 'minimal' }) .drupalInstallModule('ckeditor5', true) .drupalInstallModule('field_ui');
in the before section and then go to the module screen and see if they are installed. Because drupalInstallModule should be chainable.
- π¬π§United Kingdom longwave UK
Seems to be because drupalInstallModule() does not wait for anything. If you remove this:
.perform(() => { if (typeof callback === 'function') { callback.call(self); } })
then field_ui is installed *after* the test runs!
If I add
.waitForElementPresent('body')
before or after.perform()
then it seems to pass consistently. - π³π±Netherlands Spokje
Fun stuff, I've been working with Nightwatch a bit lately, and honestly I don't see an added value for it in the Drupal sphere.
It's great when you don't have
FunctionalJavascriptTest
, but we _do_ have that.For me it's buggy, and not well documented nor maintained.
The only plus sides of it I can think of is that it works in JavaScript and we have the a11y tests in there.
Downsides of those are that we don't have a vast user/contributor base of JavaScript savy people and most of the a11y tests in Nightwatch are commented out, waiting for someone to fix them. Not much traction on that, or I am looking in the wrong issues.I seriously think we might want to consider if we want to put any more effort into it, any version higher then we're using now locks up when using multiple workers. Not something I look forward to solve...
It might be time to consider if we want to convert every Nightwatch test to a FunctionalJavascriptTest and move away from it.
Disclaimer: n=1, I am not a JavaScript developer, etc. etc.
- πΊπΈUnited States smustgrave
To add to that, think why a lot of people probably don't contribute it's very difficult to get running locally.
- π³π±Netherlands Spokje
Back to the MR:
I think we're missing converting
core/modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5EditorHeightTest.js
to the new installing-multiple-modules-in-one-go? - Status changed to Needs work
8 months ago 7:49pm 18 January 2024 The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- π¬π§United Kingdom catch
Fun stuff, I've been working with Nightwatch a bit lately, and honestly I don't see an added value for it in the Drupal sphere.
I am wondering when the last time was we added any test coverage vs. dealing with nightwatch failures. Should probably open a new issue to discuss whether we want to think about removing it.
- π³π±Netherlands Spokje
I am wondering when the last time was we added any test coverage vs. dealing with nightwatch failures.
I know @WimLeers uses Nightwatch frequently both in core ckeditor5 and some of "his" contrib modules.
Besides that: Not much traction I can see.
Should probably open a new issue to discuss whether we want to think about removing it.
Even when the outcome is "We need it, don't remove it!", we at the very least have an indication that it's used (and loved) and maybe even the amount of users/lovers.
I really believe Nightwatch is great if you don't have any FE-testing in place, but IMHO it's on all fronts outclassed by our FunctionalJavascript test suite.
TLDR; O yes, please open such an issue.
- π¬π§United Kingdom alexpott πͺπΊπ
Re removing Nightwatch - it is doing somethings our PHP based JS testing framework cannot do.
- usability testing use Axe - see core/tests/Drupal/Nightwatch/Tests/a11yTestDefault.js
- JS unit testing - see core/modules/ckeditor5/tests/src/Nightwatch/Tests/drupalHtmlBuilderTest.js
- π³π±Netherlands Spokje
Thanks @alexpott.
I'll happily work around it :)