From 260b1e16a3ae13fecb5a96c2aeef342a634749f2 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Thu, 1 Nov 2018 13:50:27 +0000 Subject: [PATCH] Bug 1467712 - Fail if SimpleTest ok() is called with more than 2 arguments;r=Standard8 Depends on D10417 Differential Revision: https://phabricator.services.mozilla.com/D10418 --HG-- extra : moz-landing-system : lando --- .../content/test/forms/browser_selectpopup.js | 2 +- .../content/test/general/browser_bug521216.js | 10 +++---- .../mochitest/browserElement_LoadEvents.js | 8 ++++-- .../mochikit/tests/SimpleTest/SimpleTest.js | 24 ++++++++++++----- .../BrowserTestUtils/ContentTask.jsm | 2 +- testing/mochitest/browser-test.js | 16 +++++++++--- .../Harness_sanity/test_sanitySimpletest.html | 13 +++++----- .../mochitest/tests/SimpleTest/SimpleTest.js | 26 +++++++++++++------ .../lib/configs/browser-test.js | 1 + 9 files changed, 68 insertions(+), 34 deletions(-) diff --git a/browser/base/content/test/forms/browser_selectpopup.js b/browser/base/content/test/forms/browser_selectpopup.js index 64f1d7297f04..44c6fbb14890 100644 --- a/browser/base/content/test/forms/browser_selectpopup.js +++ b/browser/base/content/test/forms/browser_selectpopup.js @@ -499,7 +499,7 @@ async function performLargePopupTests(win) { // Dragging above the popup scrolls it up. EventUtils.synthesizeMouseAtPoint(popupRect.left + 20, popupRect.top - 20, { type: "mousemove" }, win); - ok(selectPopup.scrollBox.scrollTop, scrollPos, "scroll position at drag up from option"); + is(selectPopup.scrollBox.scrollTop, scrollPos, "scroll position at drag up from option"); scrollPos = selectPopup.scrollBox.scrollTop; EventUtils.synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 25, { type: "mouseup" }, win); diff --git a/browser/base/content/test/general/browser_bug521216.js b/browser/base/content/test/general/browser_bug521216.js index eda6ab4a8617..566ca8863461 100644 --- a/browser/base/content/test/general/browser_bug521216.js +++ b/browser/base/content/test/general/browser_bug521216.js @@ -11,7 +11,7 @@ function test() { BrowserTestUtils.addTab(gBrowser, "data:text/html,"); } -function record(aName) { +function recordEvent(aName) { info("got " + aName); if (!actual.includes(aName)) actual.push(aName); @@ -31,20 +31,20 @@ function record(aName) { function TabOpen(aEvent) { if (aEvent.target == tab) - record("TabOpen"); + recordEvent("TabOpen"); } var progressListener = { onLocationChange: function onLocationChange(aBrowser) { if (aBrowser == tab.linkedBrowser) - record("onLocationChange"); + recordEvent("onLocationChange"); }, onStateChange: function onStateChange(aBrowser) { if (aBrowser == tab.linkedBrowser) - record("onStateChange"); + recordEvent("onStateChange"); }, onLinkIconAvailable: function onLinkIconAvailable(aBrowser) { if (aBrowser == tab.linkedBrowser) - record("onLinkIconAvailable"); + recordEvent("onLinkIconAvailable"); }, }; diff --git a/dom/browser-element/mochitest/browserElement_LoadEvents.js b/dom/browser-element/mochitest/browserElement_LoadEvents.js index 9d4ed3c71b15..fa36b3f650d9 100644 --- a/dom/browser-element/mochitest/browserElement_LoadEvents.js +++ b/dom/browser-element/mochitest/browserElement_LoadEvents.js @@ -38,7 +38,11 @@ function runTest() { seenLocationChange = true; ok(seenLoadStart, 'Location change after load start.'); ok(!seenLoadEnd, 'Location change before load end.'); - ok(e.detail.url, browserElementTestHelpers.emptyPage1, "event's reported location"); + // XXX: Switched to from ok() to todo_is() in Bug 1467712. Follow up in 1503862 + // Fails with: event's reported location - + // got "http://example.com/tests/dom/browser-element/mochitest/file_browserElement_LoadEvents.html", + // expected "http://example.com/tests/dom/browser-element/mochitest/file_empty.html" + todo_is(e.detail.url, browserElementTestHelpers.emptyPage1, "event's reported location"); } function loadend(e) { @@ -91,7 +95,7 @@ function runTest2() { seenLocationChange = true; ok(seenLoadStart, 'Location change after load start.'); ok(!seenLoadEnd, 'Location change before load end.'); - ok(e.detail.url, browserElementTestHelpers.emptyPage2, "event's reported location"); + is(e.detail.url, browserElementTestHelpers.emptyPage2, "event's reported location"); }); iframe.addEventListener('mozbrowserloadend', function(e) { diff --git a/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js b/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js index 73709775dc44..53c2a87939dd 100644 --- a/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js +++ b/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js @@ -33,7 +33,16 @@ SimpleTest._stopOnLoad = true; /** * Something like assert. **/ -SimpleTest.ok = function (condition, name, diag) { +SimpleTest.ok = function (condition, name) { + if (arguments.length > 2) { + const diag = "Too many arguments passed to `ok(condition, name)`"; + SimpleTest.record(false, name, diag); + } else { + SimpleTest.record(condition, name); + } +} + +SimpleTest.record = function (condition, name, diag) { var test = {'result': !!condition, 'name': name, 'diag': diag || ""}; if (SimpleTest._logEnabled) SimpleTest._logResult(test, "TEST-PASS", "TEST-UNEXPECTED-FAIL"); @@ -45,12 +54,12 @@ SimpleTest.ok = function (condition, name, diag) { **/ SimpleTest.is = function (a, b, name) { var repr = MochiKit.Base.repr; - SimpleTest.ok(a == b, name, "got " + repr(a) + ", expected " + repr(b)); + SimpleTest.record(a == b, name, "got " + repr(a) + ", expected " + repr(b)); }; SimpleTest.isnot = function (a, b, name) { var repr = MochiKit.Base.repr; - SimpleTest.ok(a != b, name, "Didn't expect " + repr(a) + ", but got it."); + SimpleTest.record(a != b, name, "Didn't expect " + repr(a) + ", but got it."); }; // --------------- Test.Builder/Test.More todo() ----------------- @@ -424,11 +433,11 @@ SimpleTest.isa = function (object, clas) { }; if ( parent.SimpleTest && parent.runAJAXTest ) (function(){ - var oldOK = SimpleTest.ok; + var oldRecord = SimpleTest.record; - SimpleTest.ok = function(condition, name, diag) { - parent.SimpleTest.ok( condition, name, diag ); - return oldOK( condition, name, diag ); + SimpleTest.record = function(condition, name, diag, stack) { + parent.SimpleTest.record( condition, name, diag, stack ); + return oldRecord( condition, name, diag, stack ); }; var oldFinish = SimpleTest.finish; @@ -441,6 +450,7 @@ if ( parent.SimpleTest && parent.runAJAXTest ) (function(){ // Global symbols: var ok = SimpleTest.ok; +var record = SimpleTest.record; var is = SimpleTest.is; var isnot = SimpleTest.isnot; var todo = SimpleTest.todo; diff --git a/testing/mochitest/BrowserTestUtils/ContentTask.jsm b/testing/mochitest/BrowserTestUtils/ContentTask.jsm index 1267427fc9ac..69e8a56cc628 100644 --- a/testing/mochitest/BrowserTestUtils/ContentTask.jsm +++ b/testing/mochitest/BrowserTestUtils/ContentTask.jsm @@ -113,7 +113,7 @@ var ContentMessageListener = { } } else if (aMessage.name == "content-task:test-result") { let data = aMessage.data; - ContentTask._testScope.ok(data.condition, data.name, null, data.stack); + ContentTask._testScope.record(data.condition, data.name, null, data.stack); } else if (aMessage.name == "content-task:test-info") { ContentTask._testScope.info(aMessage.data.name); } else if (aMessage.name == "content-task:test-todo") { diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js index 129f89cfee47..f25324186eb8 100644 --- a/testing/mochitest/browser-test.js +++ b/testing/mochitest/browser-test.js @@ -17,7 +17,7 @@ ChromeUtils.defineModuleGetter(this, "ContentSearch", "resource:///modules/ContentSearch.jsm"); const SIMPLETEST_OVERRIDES = - ["ok", "is", "isnot", "todo", "todo_is", "todo_isnot", "info", "expectAssertions", "requestCompleteLog"]; + ["ok", "record", "is", "isnot", "todo", "todo_is", "todo_isnot", "info", "expectAssertions", "requestCompleteLog"]; setTimeout(testInit, 0); @@ -1289,7 +1289,15 @@ function testScope(aTester, aTest, expected) { aTest.allowFailure = expected == "fail"; var self = this; - this.ok = function test_ok(condition, name, ex, stack) { + this.ok = function test_ok(condition, name) { + if (arguments.length > 2) { + const ex = "Too many arguments passed to ok(condition, name)`."; + self.record(false, name, ex); + } else { + self.record(condition, name); + } + }; + this.record = function test_record(condition, name, ex, stack) { aTest.addResult(new testResult({ name, pass: condition, ex, stack: stack || Components.stack.caller, @@ -1297,11 +1305,11 @@ function testScope(aTester, aTest, expected) { })); }; this.is = function test_is(a, b, name) { - self.ok(a == b, name, "Got " + a + ", expected " + b, false, + self.record(a == b, name, "Got " + a + ", expected " + b, false, Components.stack.caller); }; this.isnot = function test_isnot(a, b, name) { - self.ok(a != b, name, "Didn't expect " + a + ", but got it", false, + self.record(a != b, name, "Didn't expect " + a + ", but got it", false, Components.stack.caller); }; this.todo = function test_todo(condition, name, ex, stack) { diff --git a/testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html b/testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html index 859e07b5dd58..8fff170bd447 100644 --- a/testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html +++ b/testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html @@ -23,20 +23,21 @@ function starttest() { function() { //test log info("Logging some info") - + //basic usage - ok(true, "test ok", "This should be true"); + ok(true, "test ok"); + SimpleTest.record(true, "test ok", "diagnostic information"); is(0, 0, "is() test failed"); isnot(0, 1, "isnot() test failed"); - + //todo tests todo(false, "test todo", "todo() test should not pass"); todo_is(false, true, "test todo_is"); todo_isnot(true, true, "test todo_isnot"); - + //misc SimpleTest.requestLongerTimeout(1); - + //note: this test may alter runtimes as it waits var check = false; $('textB').focus(); @@ -55,7 +56,7 @@ function starttest() { manipulateElements(); } ); - + //use helper functions function manipulateElements() { diff --git a/testing/mochitest/tests/SimpleTest/SimpleTest.js b/testing/mochitest/tests/SimpleTest/SimpleTest.js index 9f7c12740169..2da5e4b0c736 100644 --- a/testing/mochitest/tests/SimpleTest/SimpleTest.js +++ b/testing/mochitest/tests/SimpleTest/SimpleTest.js @@ -267,8 +267,16 @@ SimpleTest.setExpected(); /** * Something like assert. **/ -SimpleTest.ok = function (condition, name, diag, stack = null) { +SimpleTest.ok = function (condition, name) { + if (arguments.length > 2) { + const diag = "Too many arguments passed to `ok(condition, name)`"; + SimpleTest.record(false, name, diag); + } else { + SimpleTest.record(condition, name); + } +}; +SimpleTest.record = function (condition, name, diag, stack) { var test = {'result': !!condition, 'name': name, 'diag': diag}; if (SimpleTest.expected == 'fail') { if (!test.result) { @@ -309,19 +317,19 @@ SimpleTest.is = function (a, b, name) { // Be lazy and use Object.is til we want to test a browser without it. var pass = Object.is(a, b); var diag = pass ? "" : "got " + repr(a) + ", expected " + repr(b) - SimpleTest.ok(pass, name, diag); + SimpleTest.record(pass, name, diag); }; SimpleTest.isfuzzy = function (a, b, epsilon, name) { var pass = (a >= b - epsilon) && (a <= b + epsilon); var diag = pass ? "" : "got " + repr(a) + ", expected " + repr(b) + " epsilon: +/- " + repr(epsilon) - SimpleTest.ok(pass, name, diag); + SimpleTest.record(pass, name, diag); }; SimpleTest.isnot = function (a, b, name) { var pass = !Object.is(a, b); var diag = pass ? "" : "didn't expect " + repr(a) + ", but got it"; - SimpleTest.ok(pass, name, diag); + SimpleTest.record(pass, name, diag); }; /** @@ -1577,9 +1585,9 @@ SimpleTest.isDeeply = function (it, as, name) { var stack = [{ vals: [it, as] }]; var seen = []; if ( SimpleTest._deepCheck(it, as, stack, seen)) { - SimpleTest.ok(true, name); + SimpleTest.record(true, name); } else { - SimpleTest.ok(false, name, SimpleTest._formatStack(stack)); + SimpleTest.record(false, name, SimpleTest._formatStack(stack)); } }; @@ -1602,6 +1610,7 @@ SimpleTest.isa = function (object, clas) { // Global symbols: var ok = SimpleTest.ok; +var record = SimpleTest.record; var is = SimpleTest.is; var isfuzzy = SimpleTest.isfuzzy; var isnot = SimpleTest.isnot; @@ -1631,8 +1640,9 @@ window.onerror = function simpletestOnerror(errorMsg, url, lineNumber, } if (!SimpleTest._ignoringAllUncaughtExceptions) { // Don't log if SimpleTest.finish() is already called, it would cause failures - if (!SimpleTest._alreadyFinished) - SimpleTest.ok(isExpected, message, error); + if (!SimpleTest._alreadyFinished) { + SimpleTest.record(isExpected, message, error); + } SimpleTest._expectingUncaughtException = false; } else { SimpleTest.todo(false, message + ": " + error); diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/browser-test.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/browser-test.js index 697dcb4a0863..e70726d08081 100644 --- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/browser-test.js +++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/browser-test.js @@ -43,6 +43,7 @@ module.exports = { "isnot": false, "ok": false, "privateNoteIntentionalCrash": false, + "record": false, "registerCleanupFunction": false, "requestLongerTimeout": false, "setExpectedFailuresForSelfTest": false,