Bug 1525395 - Part 2: Rewrite a Scratchpad test to eliminate some race conditions. r=jimb
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 06 Mar 2019 23:04:01 +0000
changeset 520658 e647490b3655a5518a296ab09d164085ac21eacd
parent 520657 0f09d2f3b19f54fcb70933039fe57299daf0f273
child 520659 3d326cfcd002b1c9598151ae5d39bbc9051524f3
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb
bugs1525395
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1525395 - Part 2: Rewrite a Scratchpad test to eliminate some race conditions. r=jimb I don't understand how the test ever worked. I think the idea was that each operation would result in changes to the prefs, because those prefs are the source of truth for the recent-files list. However, I don't understand why some tests would not trigger multiple observer callbacks, which should have been a huge mess. The new code doesn't observe the prefs at all. Where possible, it waits for an appropriate promise; in other places it uses `setTimeout()` to wait for the next tick, relying on the Scratchpad implementation to be done reacting by then. Since the original code was event-driven, most tests were split across two functions. Each test function had the bottom half of one test and the top half of the next test. The new code uses async/await and can therefore at least group related functionality into single cohesive test functions. But those test functions aren't as independent as they look -- most of them still depend on previous tests to set up the expected starting state. Differential Revision: https://phabricator.services.mozilla.com/D20759
devtools/client/scratchpad/test/browser_scratchpad_recent_files.js
--- a/devtools/client/scratchpad/test/browser_scratchpad_recent_files.js
+++ b/devtools/client/scratchpad/test/browser_scratchpad_recent_files.js
@@ -27,81 +27,90 @@ var gFileName03 = "file03_ForBug651942.t
 var gFileName04 = "file04_ForBug651942.tmp";
 
 // Content for the temporary files.
 var gFileContent01 = "hello.world.01('bug651942');";
 var gFileContent02 = "hello.world.02('bug651942');";
 var gFileContent03 = "hello.world.03('bug651942');";
 var gFileContent04 = "hello.world.04('bug651942');";
 
-async function startTest() {
+// Return a promise that will be resolved after one event loop turn.
+function snooze() {
+  return new Promise(resolve => setTimeout(resolve, 0));
+}
+
+async function testAddedToRecent() {
   gScratchpad = gScratchpadWindow.Scratchpad;
 
   gFile01 = await createAndLoadTemporaryFile(gFileName01, gFileContent01);
   gFile02 = await createAndLoadTemporaryFile(gFileName02, gFileContent02);
   gFile03 = await createAndLoadTemporaryFile(gFileName03, gFileContent03);
-}
 
-// Test to see if the three files we created in the 'startTest()'-method have
-// been added to the list of recent files.
-async function testAddedToRecent() {
+  // Test to see if the files we just created have been added to the list of
+  // recent files.
   lists.recentFiles01 = gScratchpad.getRecentFiles();
 
   is(lists.recentFiles01.length, 3,
      "Temporary files created successfully and added to list of recent files.");
 
   // Create a 4th file, this should clear the oldest file.
   gFile04 = await createAndLoadTemporaryFile(gFileName04, gFileContent04);
-}
 
-// We have opened a 4th file. Test to see if the oldest recent file was removed,
-// and that the other files were reordered successfully.
-function testOverwriteRecent() {
+  // Test to see if the oldest recent file was removed,
+  // and that the other files were reordered successfully.
   lists.recentFiles02 = gScratchpad.getRecentFiles();
 
   is(lists.recentFiles02[0], lists.recentFiles01[1],
      "File02 was reordered successfully in the 'recent files'-list.");
   is(lists.recentFiles02[1], lists.recentFiles01[2],
      "File03 was reordered successfully in the 'recent files'-list.");
   isnot(lists.recentFiles02[2], lists.recentFiles01[2],
         "File04: was added successfully.");
 
   // Open the oldest recent file.
-  gScratchpad.openFile(0);
-}
+  await gScratchpad.openFile(0);
 
-// We have opened the "oldest"-recent file. Test to see if it is now the most
-// recent file, and that the other files were reordered successfully.
-function testOpenOldestRecent() {
+  // Test to see if it is now the most recent file, and that the other files
+  // were reordered successfully.
   lists.recentFiles03 = gScratchpad.getRecentFiles();
 
   is(lists.recentFiles02[0], lists.recentFiles03[2],
      "File04 was reordered successfully in the 'recent files'-list.");
   is(lists.recentFiles02[1], lists.recentFiles03[0],
      "File03 was reordered successfully in the 'recent files'-list.");
   is(lists.recentFiles02[2], lists.recentFiles03[1],
      "File02 was reordered successfully in the 'recent files'-list.");
-
-  Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 0);
 }
 
-// The "devtools.scratchpad.recentFilesMax"-preference was set to zero (0).
-// This should disable the "Open Recent"-menu by hiding it (this should not
-// remove any files from the list). Test to see if it's been hidden.
-function testHideMenu() {
+async function testHideMenu() {
+  Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 0);
+
+  // Give the Scratchpad UI time to react to the pref change, via its observer.
+  // This is a race condition; to fix it, Scratchpad would need to give us some
+  // indication that it was finished responding to a pref update - perhaps by
+  // emitting an event.
+  await snooze();
+
+  // The "devtools.scratchpad.recentFilesMax"-preference was set to zero (0).
+  // This should disable the "Open Recent"-menu by hiding it (this should not
+  // remove any files from the list). Test to see if it's been hidden.
   const menu = gScratchpadWindow.document.getElementById("sp-open_recent-menu");
   ok(menu.hasAttribute("hidden"), "The menu was hidden successfully.");
-
-  Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 2);
 }
 
-// We have set the recentFilesMax-pref to one (1), this enables the feature,
-// removes the two oldest files, rebuilds the menu and removes the
-// "hidden"-attribute from it. Test to see if this works.
-function testChangedMaxRecent() {
+async function testEnableMenu() {
+  Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 2);
+
+  // Give the Scratchpad UI time to react to the pref change. This is a race
+  // condition; see the comment in testHideMenu.
+  await snooze();
+
+  // We have set the recentFilesMax pref to a nonzero value. This enables the
+  // feature, removes the oldest files, rebuilds the menu and removes the
+  // "hidden"-attribute from it. Test to see if this works.
   const menu = gScratchpadWindow.document.getElementById("sp-open_recent-menu");
   ok(!menu.hasAttribute("hidden"), "The menu is visible. \\o/");
 
   lists.recentFiles04 = gScratchpad.getRecentFiles();
 
   is(lists.recentFiles04.length, 2,
      "Two recent files were successfully removed from the 'recent files'-list");
 
@@ -112,72 +121,70 @@ function testChangedMaxRecent() {
   let correctMenuItem = false;
   if (menuitemLabel === lists.recentFiles03[2] &&
       menuitemLabel === lists.recentFiles04[1]) {
     correctMenuItem = true;
   }
 
   is(correctMenuItem, true,
      "Two recent files were successfully removed from the 'Open Recent'-menu");
+}
 
+async function testOpenDeletedFile() {
   // We now remove one file from the harddrive and use the recent-menuitem for
   // it to make sure the user is notified that the file no longer exists.
   // This is tested in testOpenDeletedFile().
   gFile04.remove(false);
 
-  // Make sure the file has been deleted before continuing to avoid
-  // intermittent oranges.
-  waitForFileDeletion();
-}
+  // Make sure the file has been deleted before continuing.
+  while (gFile04.exists()) {
+    await snooze();
+  }
+  gFile04 = null;
 
-function waitForFileDeletion() {
-  if (gFile04.exists()) {
-    executeSoon(waitForFileDeletion);
-    return;
-  }
+  // By now we should have two recent files stored in the list but one of the
+  // files should be missing on the harddrive.
+  await gScratchpad.openFile(0);
 
-  gFile04 = null;
-  gScratchpad.openFile(0);
-}
-
-// By now we should have two recent files stored in the list but one of the
-// files should be missing on the harddrive.
-function testOpenDeletedFile() {
   const doc = gScratchpadWindow.document;
   const popup = doc.getElementById("sp-menu-open_recentPopup");
 
   is(gScratchpad.getRecentFiles().length, 1,
      "The missing file was successfully removed from the list.");
   // The number of recent files stored, plus the separator and the
   // clearRecentMenuItems-item.
   is(popup.children.length, 3,
      "The missing file was successfully removed from the menu.");
   ok(gScratchpad.notificationBox.currentNotification,
      "The notification was successfully displayed.");
   is(gScratchpad.notificationBox.currentNotification.messageText.textContent,
      gScratchpad.strings.GetStringFromName("fileNoLongerExists.notification"),
      "The notification label is correct.");
-
-  gScratchpad.clearRecentFiles();
 }
 
-// We have cleared the last file. Test to see if the last file was removed,
-// the menu is empty and was disabled successfully.
-function testClearedAll() {
+async function testClearAll() {
+  gScratchpad.clearRecentFiles();
+
+  // Give the UI time to react to the recent files being cleared out. This is a
+  // race condition. The clearRecentFiles method works asynchronously, but
+  // there is no way to wait for it to finish. A single event loop turn should
+  // be good enough.
+  await snooze();
+
+  // We have cleared the last file. Test to see if the last file was removed,
+  // the menu is empty and was disabled successfully.
   const doc = gScratchpadWindow.document;
   const menu = doc.getElementById("sp-open_recent-menu");
   const popup = doc.getElementById("sp-menu-open_recentPopup");
 
   is(gScratchpad.getRecentFiles().length, 0,
      "All recent files removed successfully.");
   is(popup.children.length, 0, "All menuitems removed successfully.");
   ok(menu.hasAttribute("disabled"),
      "No files in the menu, it was disabled successfully.");
-
-  finishTest();
 }
 
 async function createAndLoadTemporaryFile(aFileName, aFileContent) {
   info(`Create file: ${aFileName}`);
 
   // Create a temporary file.
   const aFile = FileUtils.getFile("TmpD", [aFileName]);
   aFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0o666);
@@ -207,129 +214,38 @@ function checkIfMenuIsPopulated() {
   const recentFilesPlusExtra = gScratchpad.getRecentFiles().length + 2;
 
   if (expectedMenuitemCount > 2) {
     is(expectedMenuitemCount, recentFilesPlusExtra,
        "the recent files menu was populated successfully.");
   }
 }
 
-/**
- * The PreferenceObserver listens for preference changes while Scratchpad is
- * running.
- */
-var PreferenceObserver = {
-  _initialized: false,
-
-  _timesFired: 0,
-  set timesFired(aNewValue) {
-    this._timesFired = aNewValue;
-  },
-  get timesFired() {
-    return this._timesFired;
-  },
-
-  init: function PO_init() {
-    if (this._initialized) {
-      return;
-    }
-
-    this.branch = Services.prefs.getBranch("devtools.scratchpad.");
-    this.branch.addObserver("", this);
-    this._initialized = true;
-  },
-
-  async observe(message, topic, data) {
-    if (topic != "nsPref:changed") {
-      return;
-    }
-
-    if (this._inProgress) {
-      await this._inProgress;
-    }
-
-    this._inProgress = new Promise(async resolve => {
-      info(`Times fired: ${this.timesFired}`);
-      switch (this.timesFired) {
-        case 0:
-          this.timesFired = 1;
-          break;
-        case 1:
-          this.timesFired = 2;
-          break;
-        case 2:
-          this.timesFired = 3;
-          await testAddedToRecent();
-          break;
-        case 3:
-          this.timesFired = 4;
-          testOverwriteRecent();
-          break;
-        case 4:
-          this.timesFired = 5;
-          testOpenOldestRecent();
-          break;
-        case 5:
-          this.timesFired = 6;
-          testHideMenu();
-          break;
-        case 6:
-          this.timesFired = 7;
-          testChangedMaxRecent();
-          break;
-        case 7:
-          this.timesFired = 8;
-          testOpenDeletedFile();
-          break;
-        case 8:
-          this.timesFired = 9;
-          testClearedAll();
-          break;
-      }
-
-      this._inProgress = null;
-      resolve();
-    });
-  },
-
-  uninit: function PO_uninit() {
-    this.branch.removeObserver("", this);
-  },
-};
-
-function test() {
+async function test() {
   waitForExplicitFinish();
 
   registerCleanupFunction(function() {
     gFile01.remove(false);
-    gFile01 = null;
     gFile02.remove(false);
-    gFile02 = null;
     gFile03.remove(false);
-    gFile03 = null;
-    // gFile04 was removed earlier.
-    lists.recentFiles01 = null;
-    lists.recentFiles02 = null;
-    lists.recentFiles03 = null;
-    lists.recentFiles04 = null;
-    gScratchpad = null;
-
-    PreferenceObserver.uninit();
+    // If all tests passed, gFile04 was already removed, but just in case:
+    if (gFile04) {
+      gFile04.remove(false);
+    }
     Services.prefs.clearUserPref("devtools.scratchpad.recentFilesMax");
   });
 
   Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 3);
 
-  // Initiate the preference observer after we have set the temporary recent
-  // files max for this test.
-  PreferenceObserver.init();
+  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
+  const loaded = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+  BrowserTestUtils.loadURI(gBrowser, "data:text/html,<p>test recent files in Scratchpad");
+  await loaded;
+  await new Promise(openScratchpad);
 
-  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
-  BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser).then(function() {
-    openScratchpad(startTest);
-  });
+  await testAddedToRecent();
+  await testHideMenu();
+  await testEnableMenu();
+  await testOpenDeletedFile();
+  await testClearAll();
 
-  BrowserTestUtils.loadURI(gBrowser, "data:text/html,<p>test recent files in Scratchpad");
-}
-
-function finishTest() {
   finish();
 }