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.

Sign up for GitHub

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

fix: add global leaks detection on watch-run #4059

Closed
wants to merge 2 commits into from
Closed

fix: add global leaks detection on watch-run #4059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96d4767
Add global leaks detection on watch-run (#1948)
anpaul0615 Oct 11, 2019
3c610ca
change variable name
anpaul0615 Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lib/cli/watch-run.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

@Uzlopak @voxpelli and I talked about this approach: we think it's promising and gets the job done partially, but there are blocking edge cases around side effects. For example, if the first time a test is run, the test imports cached polyfills that add globals, deleting the global after the test would mean subsequent tests would fail. delete global[k] isn't something that Mocha can reasonably do.

Instead, we think a more workable approach would be to have the runner check for global leaks against the initial state of globals before all tests. I.e. keep the initialGlobals idea, but not the delete global[k] idea. Doing so would mean that necessary globals that can only be introduced exactly one won't be deleted on --watch. (does that make sense?)

One drawback of this proposed approach is that if a leak exists in a test, then is fixed, it'll still be reported on re-runs. Mocha's messaging when --check-leaks and --watch are enabled will have to note that if the leak is fixed then the user will have to restart Mocha.

So, two requests, please:

  • fix the behavior so that globals are never deleted, and instead runner checks leaks against the same initial globals every time
  • add a unit test that adding a global variable only in the first run is still available in subsequent runs

See also #4954 / #4954 (comment). Part of why we're looking at deprecating the --check-leaks feature long-term is that it isn't comprehensive enough to get many common modern use cases. The only reliable way for a test framework to achieve true encapsulation(-ish) and leak detection is with VMs the way Jest & co do. For Mocha, you'd have to build a plugin and/or use linting and/or / type checking and/or "use strict"; and/or proper ESM.

Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,17 @@ const createRerunner = (mocha, beforeRun) => {

let rerunScheduled = false;

let initialGlobals = null;

const run = () => {
try {
beforeRun();
resetMocha(mocha);

if (!initialGlobals) {
initialGlobals = Object.keys(global);
Copy link
Member

Choose a reason for hiding this comment

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

[Style] Now that Mocha doesn't need to support super old versions of IE & the like, let's switch to a Set:

Suggested change
initialGlobals = Object.keys(global);
initialGlobals = new Set(Object.keys(global));

That way the .indexOf(...) === -1 check later can become a .has. A little more idiomatic IMO.

}

runner = mocha.run(() => {
runner = null;
if (rerunScheduled) {
Expand All @@ -114,6 +121,15 @@ const createRerunner = (mocha, beforeRun) => {
};

const rerun = () => {
if (initialGlobals) {
Object.keys(global).forEach(k => {
if (initialGlobals.indexOf(k) === -1) {
delete global[k];
}
});
initialGlobals = null;
}

rerunScheduled = false;
eraseLine();
run();
Expand Down
14 changes: 14 additions & 0 deletions test/integration/fixtures/options/watch/test-global-leak.fixture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
describe('leak detection', function() {
function testMe() {
x = 123; // leak variable
}

it('should not leak', function(done) {
testMe();
done(); // just pass
});

before(function() {});

after(function() {});
Copy link
Contributor

Choose a reason for hiding this comment

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

hooks needed here?

Copy link
Author

Choose a reason for hiding this comment

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

because Runner.prototype.checkGlobals() is invoked every test-end-event and hook-end-event. (https://github.com/mochajs/mocha/blob/master/lib/runner.js#L136-L141)

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, but isn't intuitive from the test. Request: how about a quick comment explaining why these are here?

});
13 changes: 13 additions & 0 deletions test/integration/options/watch.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,19 @@ describe('--watch', function() {
expect(results[1].tests, 'to have length', 2);
});
});

it('check global variable leaks when test reruns', function() {
const testFile = path.join(this.tempDir, 'test.js');
copyFixture('options/watch/test-global-leak', testFile);

return runMochaWatch([testFile, '--check-leaks'], this.tempDir, () => {
touchFile(testFile);
}).then(results => {
expect(results, 'to have length', 2);
expect(results[0].failures, 'to have length', 1); // first leaks detection
expect(results[1].failures, 'to have length', 1); // second leaks detection
});
});
});
});

Expand Down