Upgrade to inspectpack@4. Add regression tests to catch imports of format* utils. #263

parkerziegler · 2018-11-11T18:12:36Z

@ryan-roemer @ElreyB @ianwsperber Here's a WiP PR for the inspectpack@4.0.0 migration! Don't merge yet, I'm still underway adding tests but wanted to get some 👀on things so far.

I haven't undertaken any UI updates to leverage new inspectpack in this PR, mostly because I want to ensure we can get webpack-dashboard fully operational with latest inspectpack. The focus of this PR is on testing and code coverage, and adding regression tests for our format* utils.

I'm going to drop comments in the diff below around some things I've uncovered while testing. Would appreciate any initial feedback you have!

parkerziegler · 2018-11-11T18:17:23Z

test/dashboard/index.spec.js

+        };
+
+        expect(dashboard.setProgress(data)).to.not.throw;
+        expect(dashboard.progressbar.setProgress).to.have.been.calledOnce;


I wanted to make these asserts more firm, i.e. to.have.been.calledWith, but I'm hitting the good ol' JS-floating-point-math-is-horrible thing. The crux of the issue is that a float like 0.57 gets evaluated in our calculations in this method as 0.56999999997 (or something of the like), and our rounding setup returns 0.56 / 56%. Not sure if we want to update our rounding logic in this method, but wanted to call out that this is happening. I don't like the idea of writing:

const data = { value: 0.57 }; expect(dashboard.progressbar.setProgress).to.have.been.calledWith(0.56);

parkerziegler · 2018-11-11T18:22:41Z

test/dashboard/index.spec.js

+      expect(dashboard.screen.render).to.have.been.called;
+    });
+
+    it("can setProblems", () => {


So I'm having a hard time getting formatProblems import properly tested. The issue is that setProblems, which is the only method that uses formatProblems, doesn't actually invoke formatProblems. Rather, formatProblems is called in a function that gets assigned to a particular key in our problemsItems collection. So the function is created but never actually run in our current tests, so that import, as far as I can tell, never gets tested. If I change the import syntax on that util, all tests still pass 😬 So, not much of a regression test 😞 Let me know if any of you have a thought on a way to solve this.

I'm looking at this and will try to get to a bit more by sometime on Tuesday...

I think something like this may get you where you want to be:

diff --git a/test/dashboard/index.spec.js b/test/dashboard/index.spec.js index 6c9dd5b..4cf3a98 100644 --- a/test/dashboard/index.spec.js +++ b/test/dashboard/index.spec.js @@ -1,8 +1,9 @@ "use strict"; const chalk = require("chalk"); -require("../base.spec"); +const base = require("../base.spec"); +const blessed = require("blessed"); const Dashboard = require("../../dashboard"); describe("dashboard", () => { @@ -122,6 +123,21 @@ describe("dashboard", () => { }); it("can setProblems", () => { + // Override ListBar fakes from what we do in `base.spec.js`. + // Note that these are **already** stubbed. We're not monkey-patching blessed. + blessed.listbar.returns({ + selected: "selected", + setLabel: base.sandbox.spy(), + setProblems: base.sandbox.spy(), + selectTab: base.sandbox.spy(), + setItems: base.sandbox.stub().callsFake((obj) => { + // Naively simulate what setItems would do calling each object key. + Object.keys(obj).forEach((key) => obj[key]()); + }) + }); + + // Discard generic dashboard, create a new one with adjusted mocks. + dashboard = new Dashboard(); + const data = { value: { duplicates: {

We're really pushing the boundaries of how much to mock, but here I think it may make sense because we really want to ensure those imports are (1) happening and (2) used.

Let me know if you need tips on getting even further in to things like asserting on what's called within formatProblems or whatnot...

ryan-roemer · 2018-11-12T07:39:45Z

test/plugin/index.spec.js

+
+      plugin.observeMetrics({ toJson }).subscribe({
+        next: () => {},
+        error: () => {},


GLOBAL TIP: Use base.sandbox.spy() in place of () => {}. It can be asserted on and it's more clear your using a test double.

ryan-roemer · 2018-11-12T07:40:20Z

test/plugin/index.spec.js

@@ -1,5 +1,8 @@
 "use strict";

+const sinon = require("sinon");


GLOBAL: Don't use sinon as a best practice. Instead get spys and stubs from base.sandbox.

parkerziegler · 2018-11-14T19:36:03Z

@ElreyB here are some additional things I'd like to see:

Update version to range in format-versions.
Add tests for format-versions. Mostly we want some guarantee that our templates match what we expect, given a valid versions object. Let me know if you need help creating a mock for those tests.
Add tests for other, untested format-* modules, i.e. format-duplicates, format-problems, and error-serialization.

I think that's about the scope I'd want to see in this PR. I'm also running into issues getting this PR'ed code to run against our inspectpack fixture, so any time you have to investigate that would be great!

ElreyB · 2018-11-19T00:18:17Z

test/utils/format-versions.spec.js

+  context("when package are present", () => {
+    it("should return a handlebar compile template", () => {
+      const data = {
+        packages: {


This may be too robust but I want to make sure it hit the pkgNamePath function.

parkerziegler · 2018-11-20T06:09:21Z

@ryan-roemer This should be ready for re-review. I rebased it down to one commit already (force pushed), so you'll need a clean branch of this locally to test. Let me know if there's anything else you'd like to see in this PR.

ryan-roemer · 2018-11-20T21:16:34Z

test/dashboard/index.spec.js

+      and percent is falsy`, () => {
+        data.value = null;
+
+        expect(dashboard.setProgress(data)).to.not.throw;


GLOBAL:

These have to be of the format of "something to call", so:

expect(() => { dashboard.setProgress(data); }).to.not.throw;

unless dashboard.setProgress(data) returns a function to execute, which I don't believe it does.

Note: I was adding comments to everything I saw as "Same comment here.", but there are a lot, so just assume I did that 😛

ryan-roemer · 2018-11-20T21:16:53Z

test/dashboard/index.spec.js

+      ];
+
+      it("can setSizes", () => {
+        expect(dashboard.setSizes(data)).to.not.throw;


Same comment here.

ryan-roemer · 2018-11-20T21:16:59Z

test/dashboard/index.spec.js

+        mockSetItems();
+        // Discard generic dashboard, create a new one with adjusted mocks.
+        dashboard = new Dashboard();
+        expect(dashboard.setSizes(data)).to.not.throw;


Same comment here.

ryan-roemer · 2018-11-20T21:17:09Z

test/dashboard/index.spec.js

+      const err = "error";
+
+      it("can setSizesError", () => {
+        expect(dashboard.setSizesError(err)).to.not.throw;


Same comment here.

ryan-roemer · 2018-11-20T21:17:17Z

test/dashboard/index.spec.js

+      };
+
+      it("can setProblems", () => {
+        expect(dashboard.setProblems(data)).to.not.throw;


Same comment here.

ryan-roemer · 2018-11-20T21:17:23Z

test/dashboard/index.spec.js

+        // Discard generic dashboard, create a new one with adjusted mocks.
+
+        dashboard = new Dashboard();
+        expect(dashboard.setProblems(data)).to.not.throw;


Same comment here.

ryan-roemer · 2018-11-20T21:19:17Z

test/plugin/index.spec.js

+        tapAsync: base.sandbox.stub() // this is us in webpack-dashboard
+      };
+      compiler = {
+        // TODO: ONLY WEBPACK4, but that's what we have in devDeps


Maybe just convert this to a full explanatory comment and remove the TODO. I don't thing we necessarily need to run these unit tests through the different versions...

ryan-roemer · 2018-11-20T21:21:02Z

test/plugin/index.spec.js

+    });
+
+    it("should serialize errors when encountered", () => {
+      base.sandbox.stub(inspectpack, "actions").rejects;


Doesn't this need to be:

base.sandbox.stub(inspectpack, "actions").rejects();

?

If so, is this test working correctly?

ryan-roemer

Great work!!!

I've got lots of comments everywhere, but there mostly straightforward I think, so no re-review needed unless you want one.

Also, quick note that I'm on vacation with free time only here and there, so possibly won't get next opportunity to respond until next week. Feel free to clean up "good enough" and merge here if there are minor outstanding things for me, and we can take up together in follow-on work once I'm back next week.

parkerziegler requested review from ElreyB, ryan-roemer and ianwsperber November 11, 2018 18:12

parkerziegler commented Nov 11, 2018

View reviewed changes

ryan-roemer reviewed Nov 12, 2018

View reviewed changes

ElreyB reviewed Nov 19, 2018

View reviewed changes

parkerziegler force-pushed the task/update-to-inspectpack-4 branch from 0b6b7e8 to 3cc3f28 Compare November 20, 2018 06:07

parkerziegler changed the title ~~[WiP] Start adding regression tests to catch imports of format* utils.~~ Upgrade to inspectpack@4. Add regression tests to catch imports of format* utils. Nov 20, 2018

ryan-roemer reviewed Nov 20, 2018

View reviewed changes

ryan-roemer approved these changes Nov 20, 2018

View reviewed changes

parkerziegler force-pushed the task/update-to-inspectpack-4 branch 2 times, most recently from b94b21f to 420bd67 Compare November 24, 2018 18:01

Upgrade to inspectpack 4; add regression tests.

09cc396

parkerziegler force-pushed the task/update-to-inspectpack-4 branch from 420bd67 to 09cc396 Compare November 24, 2018 23:50

parkerziegler merged commit ab047f9 into master Nov 25, 2018

parkerziegler deleted the task/update-to-inspectpack-4 branch November 25, 2018 00:23

parkerziegler mentioned this pull request Nov 25, 2018

upgrade randomatic for yarn.lock vulnerability #262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to inspectpack@4. Add regression tests to catch imports of format* utils. #263

Upgrade to inspectpack@4. Add regression tests to catch imports of format* utils. #263

parkerziegler commented Nov 11, 2018

parkerziegler Nov 11, 2018 •

edited

Loading

parkerziegler Nov 11, 2018

ryan-roemer Nov 11, 2018

ryan-roemer Nov 12, 2018 •

edited

Loading

ryan-roemer Nov 12, 2018

ryan-roemer Nov 12, 2018

parkerziegler commented Nov 14, 2018 •

edited by ElreyB

Loading

ElreyB Nov 19, 2018

parkerziegler commented Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer Nov 20, 2018

ryan-roemer left a comment

Upgrade to inspectpack@4. Add regression tests to catch imports of format* utils. #263

Upgrade to inspectpack@4. Add regression tests to catch imports of format* utils. #263

Conversation

parkerziegler commented Nov 11, 2018

parkerziegler Nov 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryan-roemer Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerziegler commented Nov 14, 2018 • edited by ElreyB Loading

Choose a reason for hiding this comment

parkerziegler commented Nov 20, 2018

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

ryan-roemer left a comment

Choose a reason for hiding this comment

parkerziegler Nov 11, 2018 •

edited

Loading

ryan-roemer Nov 12, 2018 •

edited

Loading

parkerziegler commented Nov 14, 2018 •

edited by ElreyB

Loading