Merge pull request webpack#6176 from mikegreiling/fix-no-fail-on-child-compilation-error

sokra · web-flow · commit b059e07cf90d · 2018-01-04T20:19:57.000+01:00
Fix issue in which webpack exits with code 0 when errors exist in child compilations
diff --git a/lib/NoEmitOnErrorsPlugin.js b/lib/NoEmitOnErrorsPlugin.js
@@ -7,12 +7,12 @@
 class NoEmitOnErrorsPlugin {
 	apply(compiler) {
 		compiler.plugin("should-emit", (compilation) => {
-			if(compilation.errors.length > 0)
+			if(compilation.getStats().hasErrors())
 				return false;
 		});
 		compiler.plugin("compilation", (compilation) => {
 			compilation.plugin("should-record", () => {
-				if(compilation.errors.length > 0)
+				if(compilation.getStats().hasErrors())
 					return false;
 			});
 		});
diff --git a/lib/Stats.js b/lib/Stats.js
@@ -50,11 +50,13 @@ class Stats {
 	}
 
 	hasWarnings() {
-		return this.compilation.warnings.length > 0;
+		return this.compilation.warnings.length > 0 ||
+			this.compilation.children.some(child => child.getStats().hasWarnings());
 	}
 
 	hasErrors() {
-		return this.compilation.errors.length > 0;
+		return this.compilation.errors.length > 0 ||
+			this.compilation.children.some(child => child.getStats().hasErrors());
 	}
 
 	// remove a prefixed "!" that can be specified to reverse sort order
diff --git a/test/README.md b/test/README.md
@@ -35,7 +35,7 @@ If you are trying to solve a bug which is reproducible when x and y properties a
 
 In addition to an `index.js`, these configCases require a `webpack.config.js` is located inside of your test suite. This will run this specific config through `webpack` just as you were building individually. They will use the same loading/bundling technique of your `it()` tests, however you now have a more specific config use cases that you can write even before you start coding.
 
-#### statsCases (`Stats.test.js`)
+#### statsCases (`StatsTestCases.test.js`)
 Stats cases are similar to configCases except specifically focusing on the `expected` output of your stats. Instead of writing to the console, however the output of stats will be written to disk.
 
 By default, the "expected" outcome is a pain to write by hand so instead when statsCases are run the following happens:
diff --git a/test/Stats.unittest.js b/test/Stats.unittest.js
@@ -10,13 +10,15 @@ describe("Stats", () => {
 		describe("does have", () => {
 			it("hasErrors", () => {
 				const mockStats = new Stats({
+					children: [],
 					errors: ["firstError"],
 					hash: "1234"
 				});
 				mockStats.hasErrors().should.be.ok();
 			});
 			it("hasWarnings", () => {
 				const mockStats = new Stats({
+					children: [],
 					warnings: ["firstError"],
 					hash: "1234"
 				});
@@ -26,19 +28,49 @@ describe("Stats", () => {
 		describe("does not have", () => {
 			it("hasErrors", () => {
 				const mockStats = new Stats({
+					children: [],
 					errors: [],
 					hash: "1234"
 				});
 				mockStats.hasErrors().should.not.be.ok();
 			});
 			it("hasWarnings", () => {
 				const mockStats = new Stats({
+					children: [],
 					warnings: [],
 					hash: "1234"
 				});
 				mockStats.hasWarnings().should.not.be.ok();
 			});
 		});
+		describe("children have", () => {
+			it("hasErrors", () => {
+				const mockStats = new Stats({
+					children: [{
+						getStats: () => new Stats({
+							errors: ["firstError"],
+							hash: "5678"
+						}),
+					}],
+					errors: [],
+					hash: "1234"
+				});
+				mockStats.hasErrors().should.be.ok();
+			});
+			it("hasWarnings", () => {
+				const mockStats = new Stats({
+					children: [{
+						getStats: () => new Stats({
+							warnings: ["firstError"],
+							hash: "5678"
+						}),
+					}],
+					warnings: [],
+					hash: "1234"
+				});
+				mockStats.hasWarnings().should.be.ok();
+			});
+		});
 		it("formatError handles string errors", () => {
 			const mockStats = new Stats({
 				errors: ["firstError"],
diff --git a/test/StatsTestCases.test.js b/test/StatsTestCases.test.js
@@ -52,7 +52,7 @@ describe("StatsTestCases", () => {
 				if(/error$/.test(testName)) {
 					stats.hasErrors().should.be.equal(true);
 				} else if(stats.hasErrors()) {
-					done(new Error(stats.toJson().errors.join("\n\n")));
+					return done(new Error(stats.toJson().errors.join("\n\n")));
 				}
 
 				let toStringOptions = {
diff --git a/test/binCases/errors/child-compilation-error/index.js b/test/binCases/errors/child-compilation-error/index.js
diff --git a/test/binCases/errors/child-compilation-error/test.js b/test/binCases/errors/child-compilation-error/test.js
@@ -0,0 +1,13 @@
+"use strict";
+
+module.exports = function testAssertions(code, stdout, stderr) {
+    code.should.be.eql(2);
+
+    stdout[0].should.containEql("Hash: ");
+    stdout[1].should.containEql("Version: ");
+    stdout[2].should.containEql("Time: ");
+    stdout[5].should.containEql("./index.js");
+    stdout[8].should.containEql("ERROR in child compilation");
+
+    stderr.should.be.empty();
+};
diff --git a/test/binCases/errors/child-compilation-error/webpack.config.js b/test/binCases/errors/child-compilation-error/webpack.config.js
@@ -0,0 +1,19 @@
+"use strict";
+
+module.exports = {
+    entry: "./index",
+    output: {
+        filename: "bundle.js"
+    },
+    plugins: [{
+        apply(compiler) {
+            compiler.plugin("make", (compilation, cb) => {
+                const child = compilation.createChildCompiler("child", {});
+                child.plugin("compilation", childCompilation => {
+                    childCompilation.errors.push(new Error("child compilation"));
+                });
+                child.runAsChild(cb);
+            });
+        }
+    }]
+};
diff --git a/test/statsCases/no-emit-on-errors-plugin-with-child-error/TestChildCompilationFailurePlugin.js b/test/statsCases/no-emit-on-errors-plugin-with-child-error/TestChildCompilationFailurePlugin.js
@@ -0,0 +1,25 @@
+"use strict";
+
+var SingleEntryPlugin = require("../../../lib/SingleEntryPlugin");
+
+/**
+ * Runs a child compilation which produces an error in order to test that NoEmitErrorsPlugin
+ * recognizes errors within child compilations.
+ */
+module.exports = class TestChildCompilationFailurePlugin {
+
+	constructor(output) {
+		this.output = output;
+	}
+
+	apply(compiler) {
+		compiler.plugin("make", (compilation, cb) => {
+			const child = compilation.createChildCompiler("child", this.output);
+			child.plugin("compilation", childCompilation => {
+				childCompilation.errors.push(new Error("forced error"));
+			});
+			child.apply(new SingleEntryPlugin(compiler.options.context, compiler.options.entry, "child"));
+			child.runAsChild(cb);
+		});
+	}
+};
diff --git a/test/statsCases/no-emit-on-errors-plugin-with-child-error/expected.txt b/test/statsCases/no-emit-on-errors-plugin-with-child-error/expected.txt
@@ -0,0 +1,12 @@
+Hash: fb0670ebd6884ae658dc
+Time: Xms
+    Asset     Size  Chunks  Chunk Names
+ child.js  2.47 kB          
+bundle.js  2.47 kB       0  main
+   [0] (webpack)/test/statsCases/no-emit-on-errors-plugin-with-child-error/index.js 0 bytes {0} [built]
+Child child:
+       Asset     Size  Chunks  Chunk Names
+    child.js  2.47 kB       0  child
+       [0] (webpack)/test/statsCases/no-emit-on-errors-plugin-with-child-error/index.js 0 bytes {0} [built]
+    
+    ERROR in forced error
diff --git a/test/statsCases/no-emit-on-errors-plugin-with-child-error/index.js b/test/statsCases/no-emit-on-errors-plugin-with-child-error/index.js
diff --git a/test/statsCases/no-emit-on-errors-plugin-with-child-error/webpack.config.js b/test/statsCases/no-emit-on-errors-plugin-with-child-error/webpack.config.js
@@ -0,0 +1,17 @@
+"use strict";
+
+var NoEmitOnErrorsPlugin = require("../../../lib/NoEmitOnErrorsPlugin");
+var TestChildCompilationFailurePlugin = require("./TestChildCompilationFailurePlugin");
+
+module.exports = {
+	entry: "./index",
+	output: {
+		filename: "bundle.js"
+	},
+	plugins: [
+		new NoEmitOnErrorsPlugin(),
+		new TestChildCompilationFailurePlugin({
+			filename: "child.js"
+		})
+	]
+};

Original file line number	Diff line number	Diff line change
`@@ -50,11 +50,13 @@ class Stats {`
`50`	`50`	`}`
`51`	`51`
`52`	`52`	`hasWarnings() {`
`53`		`- return this.compilation.warnings.length > 0;`
	`53`	`+ return this.compilation.warnings.length > 0 \|\|`
	`54`	`+ this.compilation.children.some(child => child.getStats().hasWarnings());`
`54`	`55`	`}`
`55`	`56`
`56`	`57`	`hasErrors() {`
`57`		`- return this.compilation.errors.length > 0;`
	`58`	`+ return this.compilation.errors.length > 0 \|\|`
	`59`	`+ this.compilation.children.some(child => child.getStats().hasErrors());`
`58`	`60`	`}`
`59`	`61`
`60`	`62`	`// remove a prefixed "!" that can be specified to reverse sort order`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ describe("StatsTestCases", () => {`
`52`	`52`	`if(/error$/.test(testName)) {`
`53`	`53`	`stats.hasErrors().should.be.equal(true);`
`54`	`54`	`} else if(stats.hasErrors()) {`
`55`		`- done(new Error(stats.toJson().errors.join("\n\n")));`
	`55`	`+ return done(new Error(stats.toJson().errors.join("\n\n")));`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`let toStringOptions = {`