fix(run): cleanup globals if set from context #2387

straker · 2020-07-15T21:23:14Z

As discussed in the issue, just needed to clean up the set globals as jsdom creates new globals when called again.

Closes issue: #2347

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Code is reviewed for security

WilcoFiers

Please include a test to prevent regression.

stephenmathieson · 2020-07-16T13:42:44Z

Please include a test to prevent regression.

I came here just to say that ☝️

We had discussed updating the jsdom example in order to accomplish this, as the examples are run by CI.

straker · 2020-07-16T14:10:34Z

I tried updating the example, but I would have to do some convoluted stuff to make it break. For example, just trying to recreate the DOM in the it gives me an error ReferenceError: Node is not defined. The main problem is that since the examples are using axe from 3.3.1, we can't do a whole lot of major changes to the file. I could probably get it to break the with example provided in this issue, but it doesn't fit very well in a public facing example of how to use JSOM.

I also can't test this in the browser (you can't null window or document) so we can only test in node.

stephenmathieson · 2020-07-16T14:32:02Z

I tried updating the example, but I would have to do some convoluted stuff to make it break.

It's not that bad. Here's what I did:

npm run build
cd doc/examples/jsdom
# Use `"axe-core": "file://../../../"` in package.json, then
npm install jsdom@latest --save-dev
npm install
npm test

Updated JSDOM example

/* eslint-env mocha */
const axe = require('axe-core');
const { JSDOM } = require('jsdom');
const assert = require('assert');

const config = {
  rules: {
    // `color-contrast` does not work with JSDOM.
    'color-contrast': { enabled: false },
    // In this example, we are not concerned with the following rules:
    'landmark-one-main': { enabled: false },
    'page-has-heading-one': { enabled: false },
    region: { enabled: false }
  }
};

const createDOM = (working = true) => {
  const { window } = new JSDOM(`<!DOCTYPE html>
  <html lang="en">
    <head>
      <title>JSDOM Example</title>
    </head>
    <body>
    ${
      working
        ? `<div id="working">
            <label for="has-label">Label for this text field.</label>
            <input type="text" id="has-label">
          </div>`
        : `<div id="broken">
            <p>Not a label</p><input type="text" id="no-label">
          </div>`
    }
    </body>
  </html>`);
  return window;
};

describe('axe', () => {
  it('should report that good HTML is good', async () => {
    const window = createDOM(true);
    const result = await axe.run(window.document.documentElement, config);
    assert.equal(result.violations.length, 0, 'Violations is not empty');
  });

  it('should report that bad HTML is bad', async () => {
    const window = createDOM(false);
    const result = await axe.run(window.document.documentElement, config);
    assert.equal(result.violations.length, 1, 'Violations.length is not 1');
  });
});

straker · 2020-07-16T14:37:36Z

Except we determined in our last discussion of this file being both an example and a test file, that it should remain an example. So I couldn't change the main package.json file to use the axe file from the repo, so had to edit https://github.com/dequelabs/axe-core/blob/develop/doc/examples/test-examples.js#L14-L20 as a workaround so we could leave it as an example and still test the examples using the local copy of axe.

stephenmathieson · 2020-07-16T14:44:16Z

If you don't want to use the example as the test case, I'm fine with that. However, I agree with Wilco that this needs to be tested.

Would you rather create a separate JSDOM test elsewhere?

straker · 2020-07-16T14:45:44Z

I guess we'll have to so this problem of example/test doesn't prevent us from testing fixes we need to

Resolved

stephenmathieson · 2020-07-16T17:37:11Z

test/node/jsdom.js

@@ -0,0 +1,54 @@
+var axe = require('../../');
+var jsdom = require('jsdom');


Since we can use "modern JS" here, I really think we should. Thoughts on refactoring this to make it look like "today's JavaScript"?

I'd love to but we can't. The entire test dir is dictated by the eslint file in there that disallows any modern javascript... It's a bit of a pain.

That doesn't sound like a "we can't", it just sounds like some ESLint settings need to be modified. We could add test/node/.eslintrc and override the rules added there, or move that file into the directory(ies) that those rules are relevant for.

I don't consider this blocking tho. If it's too much work to mess with the ESLint stuff, I understand. We can always improve on this later.

stephenmathieson · 2020-07-16T17:38:21Z

test/node/jsdom.js

+				rules: { 'color-contrast': { enabled: false } }
+			})
+			.then(function(results) {
+				assert.equal(results.violations.length, 2);


Why check that there are 2 violations? This number is subject to change any time we add/remove rules.

Yeah, doesn't matter, just check there is a result.

stephenmathieson · 2020-07-16T17:40:18Z

test/node/jsdom.js

+						assert.equal(results.violations.length, 2);
+						done();
+					});
+			});


Without a .catch, this could cause an unhandled rejection and crash the process.

WilcoFiers · 2020-07-17T10:33:46Z

test/node/jsdom.js

+				rules: { 'color-contrast': { enabled: false } }
+			})
+			.then(function(results) {
+				assert.equal(results.violations.length, 2);


Yeah, doesn't matter, just check there is a result.

WilcoFiers · 2020-07-17T10:35:02Z

test/node/jsdom.js

+
+				var dom = new jsdom.JSDOM(domStr);
+
+				axe


If you return the promise, you can chain it, instead of nesting another promise.

Also, if we chain and return the promise, we don't have to add a .catch() or mess with done. Mocha has had support for promises for awhile now.

test/node/jsdom.js

Co-authored-by: Stephen Mathieson <[email protected]>

Resolved

fix(run): cleanup globals if set from context

4cb2af4

straker requested a review from a team as a code owner July 15, 2020 21:23

WilcoFiers previously requested changes Jul 16, 2020

View reviewed changes

test node

f22ca01

straker added 2 commits July 16, 2020 10:57

Merge branch 'develop' into jsdom

7c57ffa

build first

c3d17cd

stephenmathieson reviewed Jul 16, 2020

View reviewed changes

WilcoFiers previously requested changes Jul 17, 2020

View reviewed changes

stephenmathieson requested changes Jul 17, 2020

View reviewed changes

test/node/jsdom.js Outdated Show resolved Hide resolved

test/node/jsdom.js Outdated Show resolved Hide resolved

straker and others added 2 commits July 17, 2020 10:47

Update test/node/jsdom.js

a4d062d

Co-authored-by: Stephen Mathieson <[email protected]>

Update test/node/jsdom.js

68552c9

Co-authored-by: Stephen Mathieson <[email protected]>

stephenmathieson approved these changes Jul 17, 2020

View reviewed changes

straker merged commit d5b6931 into develop Jul 20, 2020

straker deleted the jsdom branch July 20, 2020 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(run): cleanup globals if set from context #2387

fix(run): cleanup globals if set from context #2387

straker commented Jul 15, 2020 •

edited by stephenmathieson

Loading

WilcoFiers left a comment

stephenmathieson commented Jul 16, 2020

straker commented Jul 16, 2020 •

edited

Loading

stephenmathieson commented Jul 16, 2020

straker commented Jul 16, 2020 •

edited

Loading

stephenmathieson commented Jul 16, 2020

straker commented Jul 16, 2020

stephenmathieson Jul 16, 2020

straker Jul 16, 2020

stephenmathieson Jul 16, 2020

stephenmathieson Jul 16, 2020

WilcoFiers Jul 17, 2020

stephenmathieson Jul 16, 2020

WilcoFiers Jul 17, 2020

WilcoFiers Jul 17, 2020

stephenmathieson Jul 17, 2020

		@@ -0,0 +1,54 @@
		var axe = require('../../');
		var jsdom = require('jsdom');

fix(run): cleanup globals if set from context #2387

fix(run): cleanup globals if set from context #2387

Conversation

straker commented Jul 15, 2020 • edited by stephenmathieson Loading

Reviewer checks

WilcoFiers left a comment

Choose a reason for hiding this comment

stephenmathieson commented Jul 16, 2020

straker commented Jul 16, 2020 • edited Loading

stephenmathieson commented Jul 16, 2020

straker commented Jul 16, 2020 • edited Loading

stephenmathieson commented Jul 16, 2020

straker commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straker commented Jul 15, 2020 •

edited by stephenmathieson

Loading

straker commented Jul 16, 2020 •

edited

Loading

straker commented Jul 16, 2020 •

edited

Loading