From e71e9ef311ba2ebd67044120b74801c33323a20e Mon Sep 17 00:00:00 2001 From: Kevin Lubick Date: Mon, 5 Nov 2018 07:51:40 -0500 Subject: [PATCH] [canvaskit] Add catchException everywhere This should make the logs in the bots more actionable by showing the error and trace. This also fixes the API change causing mysterious red. Bug: skia: Change-Id: I38df2bb4557041f8bdfefcae5c8d95b58e770033 Reviewed-on: https://skia-review.googlesource.com/c/168180 Reviewed-by: Kevin Lubick --- experimental/canvaskit/tests/path.spec.js | 12 +++++----- modules/pathkit/tests/effects.spec.js | 17 +++++++------- modules/pathkit/tests/path.spec.js | 28 +++++++++++------------ modules/pathkit/tests/path2d.spec.js | 20 ++++++++-------- modules/pathkit/tests/pathops.spec.js | 26 ++++++++++----------- modules/pathkit/tests/svg.spec.js | 20 ++++++++-------- modules/pathkit/tests/testReporter.js | 19 +++++++++++++++ modules/pathkit/tests/util.spec.js | 12 +++++----- 8 files changed, 86 insertions(+), 68 deletions(-) diff --git a/experimental/canvaskit/tests/path.spec.js b/experimental/canvaskit/tests/path.spec.js index 970c79dd3d4af..b05660855e338 100644 --- a/experimental/canvaskit/tests/path.spec.js +++ b/experimental/canvaskit/tests/path.spec.js @@ -59,7 +59,7 @@ describe('CanvasKit\'s Path Behavior', function() { } it('can draw a path', function(done) { - LoadCanvasKit.then(() => { + LoadCanvasKit.then(catchException(done, () => { // This is taken from example.html const surface = CanvasKit.MakeCanvasSurface('test'); expect(surface).toBeTruthy('Could not make surface') @@ -72,7 +72,7 @@ describe('CanvasKit\'s Path Behavior', function() { paint.setStrokeWidth(1.0); paint.setAntiAlias(true); paint.setColor(CanvasKit.Color(0, 0, 0, 1.0)); - paint.setStyle(CanvasKit.PaintStyle.STROKE); + paint.setStyle(CanvasKit.PaintStyle.Stroke); const path = new CanvasKit.SkPath(); path.moveTo(20, 5); @@ -107,7 +107,7 @@ describe('CanvasKit\'s Path Behavior', function() { paint.delete(); reportSurface(surface, 'path_api_example', done); - }); + })); // See CanvasKit for more tests, since they share implementation }); @@ -122,7 +122,7 @@ describe('CanvasKit\'s Path Behavior', function() { } it('can apply an effect and draw text', function(done) { - LoadCanvasKit.then(() => { + LoadCanvasKit.then(catchException(done, () => { const surface = CanvasKit.MakeCanvasSurface('test'); expect(surface).toBeTruthy('Could not make surface') if (!surface) { @@ -142,7 +142,7 @@ describe('CanvasKit\'s Path Behavior', function() { const dpe = CanvasKit.MakeSkDashPathEffect([15, 5, 5, 10], 1); paint.setPathEffect(dpe); - paint.setStyle(CanvasKit.PaintStyle.STROKE); + paint.setStyle(CanvasKit.PaintStyle.Stroke); paint.setStrokeWidth(5.0); paint.setAntiAlias(true); paint.setColor(CanvasKit.Color(66, 129, 164, 1.0)); @@ -156,6 +156,6 @@ describe('CanvasKit\'s Path Behavior', function() { path.delete(); reportSurface(surface, 'effect_and_text_example', done); - }); + })); }); }); diff --git a/modules/pathkit/tests/effects.spec.js b/modules/pathkit/tests/effects.spec.js index cb1087a8dba60..cf1e1d558e8d7 100644 --- a/modules/pathkit/tests/effects.spec.js +++ b/modules/pathkit/tests/effects.spec.js @@ -30,7 +30,7 @@ describe('PathKit\'s Path Behavior', function() { describe('Dash Path Effect', function() { it('performs dash in-place with start, stop, phase', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawStar(); let dashed = drawStar(); let notACopy = dashed.dash(10, 3, 0); @@ -46,13 +46,13 @@ describe('PathKit\'s Path Behavior', function() { dashed.delete(); phased.delete(); }); - }); + })); }); }); describe('Trim Path Effect', function() { it('performs trim in-place with start, stop, phase', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawStar(); let trimmed = drawStar(); let notACopy = trimmed.trim(0.25, .8); @@ -69,14 +69,13 @@ describe('PathKit\'s Path Behavior', function() { trimmed.delete(); complement.delete(); }); - - }); + })); }); }); describe('Transform Path Effect', function() { it('performs matrix transform in-place', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawStar(); let scaled = drawStar(); let notACopy = scaled.transform(3, 0, 0, @@ -97,13 +96,13 @@ describe('PathKit\'s Path Behavior', function() { scaled.delete(); scaled2.delete(); }); - }); + })); }); }); describe('Stroke Path Effect', function() { it('creates a stroked path in-place', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawStar(); let stroked = drawStar(); let notACopy = stroked.stroke({ @@ -131,7 +130,7 @@ describe('PathKit\'s Path Behavior', function() { stroked.delete(); rounded.delete(); }); - }); + })); }); }); diff --git a/modules/pathkit/tests/path.spec.js b/modules/pathkit/tests/path.spec.js index 852435b0838e4..7ee1aadca8581 100644 --- a/modules/pathkit/tests/path.spec.js +++ b/modules/pathkit/tests/path.spec.js @@ -26,7 +26,7 @@ describe('PathKit\'s Path Behavior', function() { } it('supports.equals()', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let path = drawSimplePath(); let otherPath = drawSimplePath(); let blank = PathKit.NewPath(); @@ -44,11 +44,11 @@ describe('PathKit\'s Path Behavior', function() { otherPath.delete(); blank.delete(); done(); - }); + })); }); it('has a copy constructor', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawSimplePath(); let copy = new PathKit.SkPath(orig); @@ -58,11 +58,11 @@ describe('PathKit\'s Path Behavior', function() { orig.delete(); copy.delete(); done(); - }); + })); }); it('has a copy method', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawSimplePath(); let copy = orig.copy(); @@ -72,11 +72,11 @@ describe('PathKit\'s Path Behavior', function() { orig.delete(); copy.delete(); done(); - }); + })); }); it('can create a copy with MakePath', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let orig = drawSimplePath(); let copy = PathKit.NewPath(orig); @@ -86,7 +86,7 @@ describe('PathKit\'s Path Behavior', function() { orig.delete(); copy.delete(); done(); - }); + })); }); }); @@ -109,7 +109,7 @@ describe('PathKit\'s Path Behavior', function() { describe('bounds and rect', function(){ it('dynamically updates getBounds()', function(done){ - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Based on test_bounds_crbug_513799 let path = PathKit.NewPath(); expect(path.getBounds()).toEqual(PathKit.LTRBRect(0, 0, 0, 0)); @@ -121,11 +121,11 @@ describe('PathKit\'s Path Behavior', function() { expect(path.getBounds()).toEqual(PathKit.LTRBRect(-5, -8, 3, 4)); path.delete(); done(); - }); + })); }); it('has getBounds() and computeTightBounds()', function(done){ - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Based on PathOpsTightBoundsIllBehaved let path = PathKit.NewPath(); path.moveTo(1, 1); @@ -138,7 +138,7 @@ describe('PathKit\'s Path Behavior', function() { path.delete(); done(); - }); + })); }); }); @@ -159,7 +159,7 @@ describe('PathKit\'s Path Behavior', function() { describe('Command arrays', function(){ it('does NOT approximates conics when dumping as toCmds', function(done){ - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let path = PathKit.NewPath(); path.moveTo(20, 120); path.arc(20, 120, 18, 0, 1.75 * Math.PI); @@ -178,7 +178,7 @@ describe('PathKit\'s Path Behavior', function() { path.delete(); done(); - }); + })); }); }); diff --git a/modules/pathkit/tests/path2d.spec.js b/modules/pathkit/tests/path2d.spec.js index f1eae3aa94803..a1bc7631eaafc 100644 --- a/modules/pathkit/tests/path2d.spec.js +++ b/modules/pathkit/tests/path2d.spec.js @@ -17,7 +17,7 @@ describe('PathKit\'s Path2D API', function() { }); it('can do everything in the Path2D API w/o crashing', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // This is taken from example.html let path = PathKit.NewPath(); @@ -85,11 +85,11 @@ describe('PathKit\'s Path2D API', function() { done(); }).catch(reportError(done)); }).catch(reportError(done)); - }); + })); }); it('can chain by returning the same object', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let path = PathKit.NewPath(); let p1 = path.moveTo(20, 5) @@ -109,11 +109,11 @@ describe('PathKit\'s Path2D API', function() { // all is well } done(); - }); + })); }); it('does not leak path objects when chaining', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // By default, we have 16 MB of memory assigned to our PathKit // library. This can be configured by -S TOTAL_MEMORY=NN // and defaults to 16MB (we likely don't need to touch this). @@ -129,7 +129,7 @@ describe('PathKit\'s Path2D API', function() { path.delete(); } done(); - }); + })); }); function drawTriangle() { @@ -142,7 +142,7 @@ describe('PathKit\'s Path2D API', function() { } it('has multiple overloads of addPath', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let basePath = PathKit.NewPath(); let otherPath = drawTriangle(); // These add path call can be chained. @@ -159,11 +159,11 @@ describe('PathKit\'s Path2D API', function() { reportPath(basePath, 'add_path_3x', done); basePath.delete(); otherPath.delete(); - }); + })); }); it('approximates arcs (conics) with quads', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let path = PathKit.NewPath(); path.moveTo(50, 120); path.arc(50, 120, 45, 0, 1.75 * Math.PI); @@ -187,7 +187,7 @@ describe('PathKit\'s Path2D API', function() { reportCanvas(canvas, 'conics_quads_approx').then(() => { done(); }).catch(reportError(done)); - }); + })); }); }); diff --git a/modules/pathkit/tests/pathops.spec.js b/modules/pathkit/tests/pathops.spec.js index 1453c2222166a..927719ab807cc 100644 --- a/modules/pathkit/tests/pathops.spec.js +++ b/modules/pathkit/tests/pathops.spec.js @@ -87,16 +87,16 @@ describe('PathKit\'s PathOps Behavior', function() { }).then((_PathKit) => { PathKit = _PathKit; PATHOP_MAP = { - 'kIntersect_SkPathOp':PathKit.PathOp.INTERSECT, - 'kDifference_SkPathOp':PathKit.PathOp.DIFFERENCE, - 'kUnion_SkPathOp': PathKit.PathOp.UNION, - 'kXOR_SkPathOp': PathKit.PathOp.XOR, - 'kXOR_PathOp': PathKit.PathOp.XOR, + 'kIntersect_SkPathOp': PathKit.PathOp.INTERSECT, + 'kDifference_SkPathOp': PathKit.PathOp.DIFFERENCE, + 'kUnion_SkPathOp': PathKit.PathOp.UNION, + 'kXOR_SkPathOp': PathKit.PathOp.XOR, + 'kXOR_PathOp': PathKit.PathOp.XOR, 'kReverseDifference_SkPathOp': PathKit.PathOp.REVERSE_DIFFERENCE, }; FILLTYPE_MAP = { - 'kWinding_FillType':PathKit.FillType.WINDING, - 'kEvenOdd_FillType':PathKit.FillType.EVENODD, + 'kWinding_FillType': PathKit.FillType.WINDING, + 'kEvenOdd_FillType': PathKit.FillType.EVENODD, 'kInverseWinding_FillType': PathKit.FillType.INVERSE_WINDING, 'kInverseEvenOdd_FillType': PathKit.FillType.INVERSE_EVENODD, }; @@ -118,11 +118,11 @@ describe('PathKit\'s PathOps Behavior', function() { } it('combines two paths with .op() and matches what we see from C++', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Test JSON created with: // ./out/Clang/pathops_unittest -J ./modules/pathkit/tests/PathOpsOp.json -m PathOpsOp$ fetch('/base/tests/PathOpsOp.json').then((r) => { - r.json().then((json)=>{ + r.json().then((json) => { expect(json).toBeTruthy(); let testNames = Object.keys(json); // Assert we loaded a non-zero amount of tests, i.e. the JSON is valid. @@ -172,15 +172,15 @@ describe('PathKit\'s PathOps Behavior', function() { done(); }); }); - }); + })); }); it('simplifies a path with .simplify() and matches what we see from C++', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Test JSON created with: // ./out/Clang/pathops_unittest -J ./modules/pathkit/tests/PathOpsSimplify.json -m PathOpsSimplify$ fetch('/base/tests/PathOpsSimplify.json').then((r) => { - r.json().then((json)=>{ + r.json().then((json) => { expect(json).toBeTruthy(); let testNames = Object.keys(json); // Assert we loaded a non-zero amount of tests, i.e. the JSON is valid. @@ -225,6 +225,6 @@ describe('PathKit\'s PathOps Behavior', function() { done(); }); }); - }); + })); }); }); diff --git a/modules/pathkit/tests/svg.spec.js b/modules/pathkit/tests/svg.spec.js index ca3afe2615ce6..ad9e6dfeff025 100644 --- a/modules/pathkit/tests/svg.spec.js +++ b/modules/pathkit/tests/svg.spec.js @@ -16,7 +16,7 @@ describe('PathKit\'s SVG Behavior', function() { }); it('can create a path from an SVG string', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { //.This is a parallelagram from // https://upload.wikimedia.org/wikipedia/commons/e/e7/Simple_parallelogram.svg let path = PathKit.FromSVGString('M 205,5 L 795,5 L 595,295 L 5,295 L 205,5 z'); @@ -34,11 +34,11 @@ describe('PathKit\'s SVG Behavior', function() { [PathKit.CLOSE_VERB]]); path.delete(); done(); - }); + })); }); it('can create an SVG string from a path', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let cmds = [[PathKit.MOVE_VERB, 205, 5], [PathKit.LINE_VERB, 795, 5], [PathKit.LINE_VERB, 595, 295], @@ -52,11 +52,11 @@ describe('PathKit\'s SVG Behavior', function() { expect(svgStr).toEqual('M205 5L795 5L595 295L5 295L205 5Z'); path.delete(); done(); - }); + })); }); it('can create an SVG string from hex values', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let cmds = [[PathKit.MOVE_VERB, "0x15e80300", "0x400004dc"], // 9.37088e-26f, 2.0003f [PathKit.LINE_VERB, 795, 5], [PathKit.LINE_VERB, 595, 295], @@ -69,11 +69,11 @@ describe('PathKit\'s SVG Behavior', function() { expect(svgStr).toEqual('M9.37088e-26 2.0003L795 5L595 295L5 295L9.37088e-26 2.0003Z'); path.delete(); done(); - }); + })); }); it('should have input and the output be the same', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let testCases = [ 'M0 0L1075 0L1075 242L0 242L0 0Z' ]; @@ -87,11 +87,11 @@ describe('PathKit\'s SVG Behavior', function() { path.delete(); } done(); - }); + })); }); it('approximates arcs (conics) with quads', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { let path = PathKit.NewPath(); path.moveTo(50, 120); path.arc(50, 120, 45, 0, 1.75 * Math.PI); @@ -106,7 +106,7 @@ describe('PathKit\'s SVG Behavior', function() { reportSVGString(svgStr, 'conics_quads_approx').then(() => { done(); }).catch(reportError(done)); - }); + })); }); }); diff --git a/modules/pathkit/tests/testReporter.js b/modules/pathkit/tests/testReporter.js index d1dcd7bc924ac..3879ec1b7bf8d 100644 --- a/modules/pathkit/tests/testReporter.js +++ b/modules/pathkit/tests/testReporter.js @@ -103,4 +103,23 @@ function setCanvasSize(ctx, width, height) { function standardizedCanvasSize(ctx) { setCanvasSize(ctx, 600, 600); +} + +// A wrapper to catch and print a stacktrace to the logs. +// Exceptions normally shows up in the browser console, +// but not in the logs that appear on the bots AND a thrown +// exception will normally cause a test to time out. +// This wrapper mitigates both those pain points. +function catchException(done, fn) { + return () => { + try { + fn() + } catch (e) { + console.log('Failed with the following error', e); + done(); + } + // We don't call done with finally because + // that would make the break the asynchronous nature + // of fn(). + } } \ No newline at end of file diff --git a/modules/pathkit/tests/util.spec.js b/modules/pathkit/tests/util.spec.js index 2161769f9b45c..92981b3b51a37 100644 --- a/modules/pathkit/tests/util.spec.js +++ b/modules/pathkit/tests/util.spec.js @@ -17,7 +17,7 @@ describe('PathKit\'s CubicMap Behavior', function() { }); it('computes YFromX correctly', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Spot check a few points const testcases = [ // input x, expected y @@ -30,10 +30,10 @@ describe('PathKit\'s CubicMap Behavior', function() { expect(PathKit.cubicYFromX(0, 0.5, 1.0, 0, tc[0])).toBeCloseTo(tc[1], 5); } done(); - }); + })); }); it('computes a point from T correctly', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Spot check a few points const testcases = [ // input t, expected x, expected y @@ -50,18 +50,18 @@ describe('PathKit\'s CubicMap Behavior', function() { expect(ans[1]).toBeCloseTo(tc[1][1]); } done(); - }); + })); }); it('does not leak, with or without cache', function(done) { - LoadPathKit.then(() => { + LoadPathKit.then(catchException(done, () => { // Run it a lot to make sure we don't leak. for (let i = 0; i < 300000; i++) { PathKit.cubicYFromX(0.1, 0.5, 0.5, 0.1, 0.1); PathKit.cubicPtFromT(0.1, 0.5, 0.5, 0.1, 0.1); } done(); - }); + })); }); }); \ No newline at end of file