fix slow test for dot reporter #3486

outsideris · 2018-09-25T07:35:14Z

Description of the Change

Although there is a slow test case for dot reporter,
coverage is missed.
Since slow duration is 2(slow: function() { return 2; }) and test duration is 2 (test.duration = 2;),
this test is not check slow test actually.

Alternate Designs

N/A

Why should this be in core?

Tests should be run as intended.

coveralls · 2018-09-25T07:46:11Z

Coverage increased (+0.1%) to 90.429% when pulling 9ec5614 on outsideris:dot-report-slow-test into 04469bf on mochajs:master.

plroebuck

You changed the test duration, but the test still don't detect "slow"-ness.

Add another assertion after this line that detects color "bright yellow" of dot.

Change PR title to "fix slow test for dot reporter".

outsideris · 2018-09-25T13:10:16Z

@plroebuck It detected "slow" because duration is 3 and slow is 2.
And it doesn't print bright yellow since this if statement.

plroebuck · 2018-09-25T21:12:48Z

These tests seem hinky to begin with. Why was the test duration ever allowed to be 1? Doubt any of our tests can execute that fast. But after you turn off colors, all passing speeds return a dot. So asserting for a dot can't prove test speed.

Should your change apply to all "on pass" tests (L69) and the line you fixed (L100) be removed instead?
Should colors be reinstated so test speed can actually be checked?

outsideris · 2018-09-26T07:28:40Z

That's the reason that we didn't find this issue even if this slow test was meaningless.
I got what you point out.

plroebuck · 2018-09-27T12:31:55Z

Although I applaud your effort in having tracked this down, I see no point in merging this PR in the name of increased coverage - without meaningful changes, it only helps further mask the underlying problems.

outsideris · 2018-09-28T15:49:22Z

I turned on useColors since comparing dots without colors is meanless for dot reporter.
All tests for dot reporter now assert result with color.

Also, I added a function color() for test purpose. At first, I used Base.color in base.js, but all test still passed if I turn on useColors or not. I want to force to check dot reporter print colors properly.

plroebuck · 2018-09-28T16:30:53Z

test/reporters/dot.spec.js

+  function color(type, str) {
+    return '\u001b[' + Base.colors[type] + 'm' + str + '\u001b[0m';
+  }
+


For simplicity of comparison, it might be easier not to be so literal.

function speedString(type, str) { return '##' + type.replace(/ /g, '-') + '_' + str; }; Base.color = speedString;

outsideris · 2018-09-28T17:10:48Z

I replaced Base.color.

plroebuck · 2018-09-28T17:36:40Z

Come to think of it, given:

if (test.duration > test.slow()) {
  test.speed = 'slow';
} else if (test.duration > test.slow() / 2) {
  test.speed = 'medium';
} else {
  test.speed = 'fast';
}

we could also do this assertion for passing tests.

expect(test.speed, 'to equal', 'fast');

outsideris · 2018-09-28T18:48:47Z

However, they are not only for speed test. We should check report type, dot in here.

plroebuck · 2018-09-28T20:34:30Z

test/reporters/dot.spec.js

          expect(stdout, 'to equal', expectedArray);
        });
      });
    });
    describe('if window width is equal to or less than 1', function() {
      describe('if test speed is fast', function() {
-        it('should return a dot', function() {
+        it('should return a grey dot', function() {
          runner = createMockRunner('pass', 'pass', null, null, test);
          Dot.call({epilogue: function() {}}, runner);
          process.stdout.write = stdoutWrite;


expect(test.speed, 'to equal', 'fast');

I don't understand what you mean and why we check only speed.

the field is set for pass events by runner in Base ctor; it's the only field set there.
test.speed assertions are in addition to your stdout assertions, not in lieu of them.
they should agree though.

plroebuck · 2018-09-28T20:35:06Z

test/reporters/dot.spec.js

@@ -80,28 +86,38 @@ describe('Dot reporter', function() {
          runner = createMockRunner('pass', 'pass', null, null, test);
          Dot.call({epilogue: function() {}}, runner);
          process.stdout.write = stdoutWrite;


expect(test.speed, 'to equal', 'fast');

plroebuck · 2018-09-28T20:35:36Z

test/reporters/dot.spec.js

-      describe('if test speed is slow', function() {
-        it('should return a dot', function() {
+      describe('if test speed is medium', function() {
+        it('should return a yellow dot', function() {
          test.duration = 2;
          runner = createMockRunner('pass', 'pass', null, null, test);
          Dot.call({epilogue: function() {}}, runner);
          process.stdout.write = stdoutWrite;


expect(test.speed, 'to equal', 'medium');

plroebuck · 2018-09-28T20:36:47Z

test/reporters/dot.spec.js

+          test.duration = 3;
+          runner = createMockRunner('pass', 'pass', null, null, test);
+          Dot.call({epilogue: function() {}}, runner);
+          process.stdout.write = stdoutWrite;


expect(test.speed, 'to equal', 'slow');

plroebuck · 2018-09-28T21:02:10Z

test/reporters/dot.spec.js

@@ -50,7 +56,7 @@ describe('Dot reporter', function() {
        runner = createMockRunner('pending', 'pending');
        Dot.call({epilogue: function() {}}, runner);
        process.stdout.write = stdoutWrite;


If we're doing stream reassignment after Dot.call(), why repeat in afterEach()?
And we should explain why it's being done within test, rather than afterwards.

function runReporter(stubSelf, runner) { // Reassign stream in order to make a copy of all reporter output var stdoutWrite = process.stdout.write; process.stdout.write = function(string, enc, callback) { stdout.push(string); stdoutWrite.call(process.stdout, string, enc, callback); }; // Invoke reporter Dot.call(stubSelf, runner); // Revert stream reassignment here so reporter output // can't be corrupted if any test assertions throw process.stdout.write = stdoutWrite; }

I will test what you provide.

plroebuck · 2018-09-28T21:23:11Z

test/reporters/dot.spec.js

  });

  afterEach(function() {
    Base.useColors = useColors;
    Base.window.width = windowWidth;
+    Base.color = color;
    process.stdout.write = stdoutWrite;


runner = undefined;

Did you mean that reset runner here before every test?

yes. runner recreated per-test. reset ensures you can't forget to do so.

outsideris · 2018-10-01T18:44:28Z

I fixed them.

plroebuck · 2018-10-02T09:30:17Z

test/reporters/dot.spec.js

-  beforeEach(function() {
-    stdout = [];
-    stdoutWrite = process.stdout.write;
+  function runReporter(stubSelf, runner) {


function runReporter(stubSelf, runner, tee) {

plroebuck · 2018-10-02T09:31:57Z

test/reporters/dot.spec.js

-    stdoutWrite = process.stdout.write;
+  function runReporter(stubSelf, runner) {
+    // Reassign stream in order to make a copy of all reporter output
+    var stdoutWrite = process.stdout.write;
    process.stdout.write = function(string, enc, callback) {
      stdout.push(string);
      stdoutWrite.call(process.stdout, string, enc, callback);


if (tee) { stdoutWrite.call(process.stdout, string, enc, callback); }

plroebuck · 2018-10-02T09:36:30Z

test/reporters/dot.spec.js

@@ -8,35 +8,50 @@ var createMockRunner = require('./helpers.js').createMockRunner;

 describe('Dot reporter', function() {
  var stdout;


var showOutput = false;

plroebuck · 2018-10-02T09:38:32Z

test/reporters/dot.spec.js

  });

  describe('on start', function() {
    it('should return a new line', function() {
      runner = createMockRunner('start', 'start');
-      Dot.call({epilogue: function() {}}, runner);
-      process.stdout.write = stdoutWrite;
+      runReporter({epilogue: function() {}}, runner);


Add showOutput argument to all runReporter invocations that follow.

runReporter({epilogue: function() {}}, runner, showOutput);

plroebuck · 2018-10-02T09:56:40Z

It looked good. But glancing job results on Travis, wondered why we would be displaying the test output (which intermixes with the job "spec" report).

So my final requested modification:

Add argument tee to runReport function
Add a describe()-level global showOutput which defaults to false
Make all invocations of runReport add showOutput as final parameter.

Now, debugging a test is still simple, but output is turned off by default.

plroebuck · 2018-10-02T10:25:56Z

test/reporters/dot.spec.js

  var runner;
  var useColors;
  var windowWidth;
+  var color;



/** * Run reporter using stream reassignment to capture output. * * @param {Object} stubSelf - Reporter-like stub instance * @param {Runner} runner - Mock instance * @param {boolean} [tee=false] - If `true`, echo captured output to screen */

outsideris · 2018-10-03T06:13:25Z

Now, debugging a test is still simple, but output is turned off by default.

It is a good idea.
We can apply this idea to other reporter tests. Modification for other reporters is in another PR is better.

plroebuck · 2018-10-03T16:22:37Z

test/reporters/dot.spec.js

  });

  afterEach(function() {
    Base.useColors = useColors;
    Base.window.width = windowWidth;
-    process.stdout.write = stdoutWrite;
+    Base.color = color;
+    runner = undefined;
  });

  describe('on start', function() {
    it('should return a new line', function() {


"should write a newline"

plroebuck · 2018-10-03T16:23:42Z

test/reporters/dot.spec.js

@@ -48,18 +73,16 @@ describe('Dot reporter', function() {
      });
      it('should return a new line and then a coma', function() {


"should write a newline followed by a comma"

plroebuck · 2018-10-03T16:24:11Z

test/reporters/dot.spec.js

-        process.stdout.write = stdoutWrite;
-        var expectedArray = ['\n  ', Base.symbols.comma];
+        runReporter({epilogue: function() {}}, runner, showOutput);
+        var expectedArray = ['\n  ', 'pending_' + Base.symbols.comma];
        expect(stdout, 'to equal', expectedArray);
      });
    });
    describe('if window width is equal to or less than 1', function() {
      it('should return a coma', function() {


"should write a comma"

plroebuck · 2018-10-03T16:26:48Z

test/reporters/dot.spec.js

@@ -78,30 +101,40 @@ describe('Dot reporter', function() {
      describe('if test speed is fast', function() {
        it('should return a new line and then a dot', function() {


"should write a newline followed by a dot"

plroebuck · 2018-10-03T16:28:33Z

test/reporters/dot.spec.js

@@ -119,18 +152,16 @@ describe('Dot reporter', function() {
      });
      it('should return a new line and then an exclamation mark', function() {


"should write a newline followed by an exclamation mark"

plroebuck · 2018-10-03T17:38:34Z

test/reporters/dot.spec.js

          expect(stdout, 'to equal', expectedArray);
        });
      });
    });
    describe('if window width is equal to or less than 1', function() {
      describe('if test speed is fast', function() {
-        it('should return a dot', function() {
+        it('should return a grey dot', function() {


"should write a grey dot"

plroebuck · 2018-10-03T17:39:02Z

test/reporters/dot.spec.js

-      describe('if test speed is slow', function() {
-        it('should return a dot', function() {
+      describe('if test speed is medium', function() {
+        it('should return a yellow dot', function() {


"should write a yellow dot"

plroebuck · 2018-10-03T17:39:28Z

test/reporters/dot.spec.js

+        });
+      });
+      describe('if test speed is slow', function() {
+        it('should return a bright yellow dot', function() {


"should write a bright yellow dot"

plroebuck · 2018-10-03T17:40:10Z

test/reporters/dot.spec.js

-        process.stdout.write = stdoutWrite;
-        var expectedArray = ['\n  ', Base.symbols.bang];
+        runReporter({epilogue: function() {}}, runner, showOutput);
+        var expectedArray = ['\n  ', 'fail_' + Base.symbols.bang];
        expect(stdout, 'to equal', expectedArray);
      });
    });
    describe('if window width is equal to or less than 1', function() {
      it('should return an exclamation mark', function() {


"should write an exclamation mark"

Signed-off-by: Outsider <[email protected]>

outsideris · 2018-10-03T18:17:55Z

I updated the test names.

plroebuck

Approved. I think the Dot reporter test specification is now an example to be emulated by the others. Sorry about the constant flow of changes, but after each iteration different things stood out.

Thanks for taking the time to get this fixed properly!

Labels: reporter, semver-patch.
LGTM!

plroebuck · 2018-10-03T20:29:52Z

On the other hand, I am starting to rethink the design of some of the code in the Base reporter. Think many of these standalone exports (e.g., Base.color()) should have been subclass instance properties/methods rather than module exports. Assuming a goal of being able to have multiple reporters for a Mocha execution, this would need change to prevent one reporter from (even inadvertently) affecting any others. For example, Dot.colors.slow = 'bright yellow'; in its ctor would have eliminated the if condition within the onPassCB event handler.

outsideris · 2018-10-08T18:00:22Z

For example, Dot.colors.slow = 'bright yellow'; in its ctor would have eliminated the if condition within the onPassCB event handler.

what ctor means?

outsideris · 2018-10-08T18:00:29Z

I will merge this if one more core members approved.

plroebuck · 2018-10-08T18:14:38Z

@outsideris ctor

outsideris · 2018-10-08T18:47:48Z

I see. Thank you.

outsideris added the qa label Sep 25, 2018

plroebuck suggested changes Sep 25, 2018

View reviewed changes

outsideris changed the title ~~fix slow test properly fot dot reporter~~ fix slow test for dot reporter" Sep 25, 2018

outsideris force-pushed the dot-report-slow-test branch from d21f868 to 114eaf1 Compare September 28, 2018 15:43

plroebuck reviewed Sep 28, 2018

View reviewed changes

outsideris force-pushed the dot-report-slow-test branch from 114eaf1 to 10caf9a Compare September 28, 2018 17:10

plroebuck reviewed Sep 28, 2018

View reviewed changes

outsideris force-pushed the dot-report-slow-test branch from 10caf9a to 3a3c8d7 Compare October 1, 2018 18:44

plroebuck reviewed Oct 2, 2018

View reviewed changes

outsideris force-pushed the dot-report-slow-test branch from 3a3c8d7 to 16a6fa2 Compare October 3, 2018 06:10

outsideris changed the title ~~fix slow test for dot reporter"~~ fix slow test for dot reporter Oct 3, 2018

plroebuck reviewed Oct 3, 2018

View reviewed changes

fix slow test properly fot dot reporter

9ec5614

Signed-off-by: Outsider <[email protected]>

outsideris force-pushed the dot-report-slow-test branch from 16a6fa2 to 9ec5614 Compare October 3, 2018 18:17

plroebuck approved these changes Oct 3, 2018

View reviewed changes

outsideris added area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Oct 8, 2018

Munter approved these changes Oct 10, 2018

View reviewed changes

Bamieh merged commit a03e23a into mochajs:master Oct 11, 2018

outsideris mentioned this pull request Oct 21, 2018

Extract runReporter to capture output under tests #3528

Merged

boneskull added this to the v6.0.0 milestone Nov 5, 2018

snyk-bot mentioned this pull request Jan 9, 2021

[Snyk] Security upgrade karma from 1.7.1 to 5.0.8 Deckhandfirststar01/mocha#26

Open

snyk-bot mentioned this pull request May 6, 2021

[Snyk] Security upgrade karma from 1.7.1 to 5.0.8 Deckhandfirststar01/mocha#31

Open

snyk-bot mentioned this pull request May 27, 2021

[Snyk] Security upgrade karma from 1.7.1 to 5.0.8 Deckhandfirststar01/mocha#32

Open

snyk-bot mentioned this pull request Jan 21, 2022

[Snyk] Security upgrade karma from 1.7.1 to 5.0.8 Deckhandfirststar01/mocha#36

Open

		@@ -8,35 +8,50 @@ var createMockRunner = require('./helpers.js').createMockRunner;

		describe('Dot reporter', function() {
		var stdout;

		@@ -48,18 +73,16 @@ describe('Dot reporter', function() {
		});
		it('should return a new line and then a coma', function() {

		@@ -78,30 +101,40 @@ describe('Dot reporter', function() {
		describe('if test speed is fast', function() {
		it('should return a new line and then a dot', function() {

		@@ -119,18 +152,16 @@ describe('Dot reporter', function() {
		});
		it('should return a new line and then an exclamation mark', function() {

fix slow test for dot reporter #3486

fix slow test for dot reporter #3486

Conversation

outsideris commented Sep 25, 2018

Description of the Change

Alternate Designs

Why should this be in core?

coveralls commented Sep 25, 2018 • edited Loading

plroebuck left a comment

Choose a reason for hiding this comment

outsideris commented Sep 25, 2018

plroebuck commented Sep 25, 2018 • edited Loading

outsideris commented Sep 26, 2018

plroebuck commented Sep 27, 2018

outsideris commented Sep 28, 2018

plroebuck Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

outsideris commented Sep 28, 2018

plroebuck commented Sep 28, 2018 • edited Loading

outsideris commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plroebuck Sep 30, 2018 • edited Loading

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

plroebuck Sep 28, 2018 • edited Loading

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

outsideris commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plroebuck Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

plroebuck commented Oct 2, 2018

Choose a reason for hiding this comment

outsideris commented Oct 3, 2018

plroebuck Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

plroebuck Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

plroebuck Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

plroebuck Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

plroebuck Oct 3, 2018 • edited Loading

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

outsideris commented Oct 3, 2018

plroebuck left a comment • edited Loading

Choose a reason for hiding this comment

plroebuck commented Oct 3, 2018

outsideris commented Oct 8, 2018

outsideris commented Oct 8, 2018

plroebuck commented Oct 8, 2018

outsideris commented Oct 8, 2018

coveralls commented Sep 25, 2018 •

edited

Loading

plroebuck commented Sep 25, 2018 •

edited

Loading

plroebuck Sep 28, 2018 •

edited

Loading

plroebuck commented Sep 28, 2018 •

edited

Loading

plroebuck Sep 30, 2018 •

edited

Loading

plroebuck Sep 28, 2018 •

edited

Loading

plroebuck Oct 2, 2018 •

edited

Loading

plroebuck Oct 3, 2018 •

edited

Loading

plroebuck Oct 3, 2018 •

edited

Loading

plroebuck Oct 3, 2018 •

edited

Loading

plroebuck Oct 3, 2018 •

edited

Loading

plroebuck Oct 3, 2018 •

edited

Loading

plroebuck left a comment •

edited

Loading