Problem/Motivation

In developing a mechanism for contrib to run nightwatch.js tests, we discovered that when it attempts to write out the filename for the console logs, some directories are not created, because the testname can sometimes have '/''s in it.

Proposed resolution

Properly clean up the testname to convert spaces to dashes, and slashes to underscores.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#19 2974553-19.patch2.98 KBMixologic
#19 interdiff-2974553-17-19.txt1.01 KBMixologic
#17 2974553-17.patch3.18 KBjustafish
#16 2974553-16.patch3.17 KBjustafish
#16 2974553-14.patch3.17 KBjustafish
#9 interdiff_8-9.txt815 bytesMixologic
#9 2974553-9.patch1.4 KBMixologic
#8 29745553-8.patch1.31 KBMixologic
#6 interdiff.txt697 bytesMixologic
#6 2974553-5.patch1.23 KBMixologic
#2 2974553-2.patch1.24 KBMixologic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mixologic created an issue. See original summary.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Active » Needs review
FileSize
2974553-2.patch1.24 KB
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Related issues: +#2973354: Enable Nightwatch Testing for Contrib

Adding a blocked drupalci issue.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Thank you @mixologic for this patch! I'm curious why you haven't used .replace(/[\s/]+/g, '-'), am I missing something?

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

@dawehner Nice one. I always forget about [ ] in regex.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
FileSize
2974553-5.patch1.23 KB
interdiff.txt697 bytes

Mostly because the slack convo sorta just ended up with that. just so the filename separated the module name from the test name. It could be just dashes too.

Tests_autodrop-JSTest_PASSED_Tue-May-22-2018-14:53:52-GMT+0000-(UTC)_console.json

vs

Tests-autodrop-JSTest_PASSED_Tue-May-22-2018-14:53:52-GMT+0000-(UTC)_console.json

maybe just the underscores separating the filenames from the status from date are all thats needed.

Your suggestion, I think, makes for simpler code vs whats there.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/Nightwatch/globals.js
@@ -32,7 +32,6 @@ module.exports = {
       let testName = browser.currentTest.name || browser.currentTest.module;

    Since testName is never reassigned, you can use const now. 

  2. +++ b/core/tests/Drupal/Nightwatch/globals.js
@@ -40,7 +39,7 @@ module.exports = {
+          fs.writeFileSync(`${resultPath}/${testName.replace(/[\s\/]+/g, '-')}_${status}_${now}_console.json`, browserlog);

    You could also replace testName here with (browser.currentTest.name || browser.currentTest.module)

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs work » Needs review
FileSize
29745553-8.patch1.31 KB

Feedback incorporated, and a browserlog to browserLog fix incorporated.

Not interdiffing since its mostly the same size as the patch.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
FileSize
2974553-9.patch1.4 KB
interdiff_8-9.txt815 bytes

One more bit of feedback incorporated : if we're cleaning strings of unwanted chars, lets do it all in the same style. Since we're cleaning both slashes and spaces out of one string, the split/join method seemed repetitive and verbose, so I opted to change the other to a regex replace.

The downside is regex readability, and "move all the things into one line". I'll leave this up to the JS maintainers to ultimately decide style choices - Im not informed enough in JS to decide one way or the other.

In any case this fix will unblock nightwatch testing in core and contrib. Right now nightwatch isnt producing any log data, so jenkins cant process the results, so although the tests are running, and you can see their results in the console log, drupalci cannot pass or fail the tests based on the outcome.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

Dang. I reversed my interdiff -> patch is going from splits to regexes.

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
CreditAttribution: justafish as a volunteer and at Lullabot commented
Status: Needs review » Reviewed & tested by the community

👍

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

Crediting @dawehner and @drpal for reviews on the issue and crediting @justafish for discussions that took place in #javascript Drupal slack.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Needs review

There's a JS style issue.

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Nightwatch/globals.js
  41:114  error  Unnecessary escape character: \/  no-useless-escape

✖ 1 problem (1 error, 0 warnings)

I think /[\s\/]+/g can be /[\s/]+/g

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Oh yeah this one is always getting me. It just not obvious at all that you can skip the escaping.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
FileSize
2974553-15.patch1.4 KB
interdiff.txt776 bytes

I wasnt sure if we were linting those files yet. On the testbots we've got

/var/lib/drupalci/workspace/jenkins-drupal_patches-59273/source/core/tests/Drupal/Nightwatch/globals.js
line 0
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

So perhaps something needs to change in the .eslintignore?

Heres a patch that fixes the escaping.

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
CreditAttribution: justafish as a volunteer and at Lullabot commented
FileSize
2974553-14.patch3.17 KB
2974553-16.patch3.17 KB

This error wasn't getting picked up when running any of the scripts in package.json as it was only running on .es6.js files

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
CreditAttribution: justafish as a volunteer and at Lullabot commented
FileSize
2974553-17.patch3.18 KB

Oops, will open up a follow up for the rogue || exit 0 stuff

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Needs work
Issue tags: +JavaScript 
  1. +++ b/core/package.json
@@ -12,9 +12,9 @@
+    "lint:core-js": "node ./node_modules/eslint/bin/eslint.js . || exit 0",

    Makes sense to drop the --ext option here since we have this handled in the .eslintignore file. 

  2. +++ b/core/tests/Drupal/Nightwatch/globals.js
@@ -31,16 +31,14 @@ module.exports = {
+          fs.writeFileSync(`${resultPath}/${(browser.currentTest.name || browser.currentTest.module).replace(/[\s/]+/g, '-')}_${status}_${now}_console.json`, browserLog);

    We should either create the variables outside this string, or move now into this and avoid creating a variable. Either way is fine with me, but lets be consistent.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
FileSize
interdiff-2974553-17-19.txt1.01 KB
2974553-19.patch2.98 KB

After fighting for seconds with these changes, I finally have a fix.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs work » Needs review

I dont know how to use the tools I help maintain.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Reviewed & tested by the community

👍 Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed 2e75397 and pushed to 8.6.x. Thanks!

  • alexpott committed 2e75397 on 8.6.x 
    Issue #2974553 by Mixologic, justafish, drpal, dawehner: Nightwatch...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.