This is the fabled merge, attachment to follow.

Change guide (check here first if you have questions about the code): * Ported places.js for supporting known state operations * windows.js changes: o Call to addEventListener() now uses the WindowWrapper's version o Added keypress() o Added add/removeEventListener() methods * Added beginnings of dtds.js to centralize dtd constants * head.js now sets up and tears down to known state (no tabs, no history, default bookmarks). The tab part can be disabled on setup for any current tests that assume tabs exist. * Removed redundant prefs.service export from prefs.js. Would prefer all services come from services.js for consistency's sake. * test_modalDialogDefaultHandler.js changed to preserve original tabs during setup. Unsure why, but the modal that pops during the test if there are no tabs isn't handled correctly by the default handler. This should be investigated, but is beyond my knowledge of the modals code; I left an // XXX for it. * Changes to browser.js: o Restored logic to openURL() that would cause it to wait by default. Can be disabled with an explicit 0 timeout. o waitForPageLoad() now supports all functionality of the original. o Added closeTab() o Added resetTabs() (closes everything, opens about:blank). * widgets.js changes: o "anon" is now an option for the maps, finds via key/val pair. o locateElem() logic significantly changed to support implicit waitFor() o Implicit waitFor() added to element access via the elem property. o exists property now honors implicit waitFor() o existsNow property added that bypasses implicit waitFor() o add/removeEventListener() added for elements * testWidgets.js and testLocationBar.js now moved into a testSmoke subdirectory, since they aren't really ported tests as much as early PoC smoketests. Plus it was inconsistent to have bare tests in what would otherwise be a directory of subdirs. * Added testBackForwardButtons-new.js, an overhaul of testBackForwardButtons for greater comprehensibility. Lots of notes in the test as to what I did. I want to do this with a handful of representative tests, though obviously we need to come to agreement on the values/techniques. * Added testStopReloadButtons.js (when this is merged, can close out bug 643490 which has been added as a dependency)

Should correct myself, known state is actually one tab w/ about:blank (it uses browser.resetTabs() to do it)

Comment on attachment 555271 [details] [diff] [review] Merge in patch form for review >+++ b/lib/api/places.js Modules which are getting used by tests themselves should not be stored under API. As discussed those have to land in /lib/ directly. How do we want to proceed with creating tests for the API? Do we want to create follow-up bugs for each of the newly integrated modules? I feel we should do this without getting lazy later. >+var driver = require('driver'); >+ // Set up an observer so we get notified when remove completes >+ var expirationSuccessful = false; >+ let expirationObserver = { >+ observe: function(aSubject, aTopic, aData) { >+ if (aTopic === TOPIC_EXPIRATION_FINISHED) >+ expirationSuccessful = true; >+ } expirationObserver is an instance so shouldn't we move the expirationSuccessful flag into it and simply name it finished? We can then access it via expirationObserver.finished. >+ services.obs.addObserver(expirationObserver, TOPIC_EXPIRATION_FINISHED, false); >+ try { >+ // Fire off the remove >+ services.browserHistory.removeAllPages(); >+ >+ // Wait for it to be finished--observer above flips the flag. >+ driver.waitFor(function () { >+ return expirationSuccessful; >+ }, "History has been removed"); Can we ensure that this task will always complete under 5s which is the default value here? See restoreDefaultBookmarks where we already have a longer timeout specified. >+ var importSuccessful = false; >+ var importObserver = { >+ observe: function (aSubject, aTopic, aData) { >+ if (aTopic === TOPIC_BOOKMARKS_RESTORE_SUCCESS) >+ importSuccessful = true; >+ } >+ } Same as above. Why do you remove the listener from the innerWindow? It should still exist so we can catch cases when the ChromeWindow gets closed by other code than us. Otherwise we will not correctly destroy e.g. a running defaultModalDialog handler as long as our wrapper instance exists. > /** >+ * Press a key for the entire window >+ */ We should try to document things on the fly when we create the code. If you wanna do this later please file a bug. >+ChromeWindowWrapper.prototype.keypress = >+function ChromeWindowWrapper_keypress(aKeyCode, aModifiers) { >+ aModifiers = aModifiers || {}; >+ return this._controller.keypress(null, aKeyCode, aModifiers); In this case you should specify this.innerWindow instead of null. Otherwise we will fail in Mozmill 2 where null currently doesn't work. Also it makes it more understandable to know where the event is getting sent to. >+ChromeWindowWrapper.prototype.addEventListener = >+function ChromeWindowWrapper_addEventListener(aType, aListener, aUseCapture, >+ aWantsUntrusted) { >+ this.innerWindow.addEventListener(aType, aListener, aUseCapture, aWantsUntrusted); >+} I think that we still leak memory in those cases. It's nothing new introduced by your code now, but in our tests we actually never close the browser window. And for each test we attach a new event handler. We call destroy() in teardown but the listener never gets freed. Lets do that in a follow-up bug. >+++ b/lib/dtds.js Even tests wouldn't use this module I'm ok in leaving it in /lib/. There can be cases and code in here is kinda specific similar to the other shared modules. >+const GLOBAL_DTD = "chrome://global/locale/global.dtd"; >+const BROWSER_DTD = "chrome://browser/locale/browser.dtd"; >+const TAB_BROWSER_DTD = "chrome://browser/locale/tabbrowser.dtd"; Do we really need the _DTD suffix? All constants in this file will be about DTDs so I don't think it's necessary. >+const TAB_BROWSER = [BROWSER_DTD, >+ TAB_BROWSER_DTD, >+ GLOBAL_DTD]; ... and you don't use it here for TAB_BROWSER. So lets get rid of those? >+ // Restore default bookmarks before the test in case last didn't tear down right. >+ // This is documented in places.js as a longish operation, but seems very >+ // short in practice. If this becomes too much a drag, it can be removed with some risk. >+ //places.restoreDefaultBookmarks(); >+ >+ // Remove all history before the test in case last didn't tear down up right >+ //places.removeAllHistory(); Why are those methods commented out? Calling those methods per default could be risky, especially for restart tests which check the history after a restart. I wonder if setup() should better get an JSON object as second parameter, so that we can let the test specify as much as possible flags and don't have to call out each of the parameters. > function teardown(aModule) { >+ // Remove all history after the test >+ places.removeAllHistory(); >+ >+ // Restore the default bookmarks after the test >+ places.restoreDefaultBookmarks(); >+ >+ // Reset tabs to known state after the test >+ aModule.browser.resetTabs(); Same here as above. Should be selective by the test, so we don't shoot ourselves in the knee with restart tests. >+++ b/lib/services.js >+ * Service to provide import and export of bookmarks in the Netscape Bookmarks HTML format. >+ * >+ * @see <a href="http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsIPlacesImportExportService.idl">nsIPlacesImportExportService</a> >+ */ >+XPCOMUtils.defineLazyGetter(services, "importexport", function () { >+ return Cc["@mozilla.org/browser/places/import-export-service;1"]. >+ getService(Ci.nsIPlacesImportExportService); For non-standard service names we should be more explicit. Otherwise you can not distinct between different components, e.g. JSON import/export. We could call it placesImportExport similar to the interface name. >+++ b/lib/tests/test_modalDialogDefaultHandler.js >- head.setup(module); >+ // Don't reset the tabs--this test doesn't succeed with the >+ // Clear Recent History modal dialog that results if you do. >+ // XXX: why not? >+ head.setup(module, false); Please file that as a new bug and add the number in here. I can investigate that. >+++ b/lib/ui/browser.js > BrowserWindow.prototype.openURL = > function BrowserWindow_openURL(aURL, aTimeout) { > this._controller.open(aURL); >- if (aTimeout) >+ if (aTimeout === undefined || aTimeout > 0) > this.waitForPageLoad(aTimeout); Wonderful. That's what I was mostly missing! >+BrowserWindow.prototype.closeTab = >+function Browser_closeTab() { >+ // Turn tab animation off for closure >+ const PREF_TABS_ANIMATE = "browser.tabs.animate"; >+ prefs.setPref("boolean", PREF_TABS_ANIMATE, false); [..] >+ finally { >+ prefs.clearUserPref(PREF_TABS_ANIMATE); >+ } You don't have to do this now but please file a bug so we safe-off the current value of the preference and don't reset it to default afterward but simply restore the value. >+ // Hit the shortcut key for closing tabs >+ var cmdKey = l10n.getEntity(dtds.TAB_BROWSER, "closeCmd.key"); >+ this.keypress(cmdKey, {accelKey: true}); As talked on IRC we want to make use of the menu per default. Lets get this done in a followup bug. >+++ b/lib/ui/widgets.js >+ var collector = this._getCollector(); >+ collector.queryAnonymousNodes(key, val); >+ >+ // No need to check for multiple; node collector only returns 1 or 0 for anon. >+ >+ if (collector.nodes.length = 1) >+ elem = new elementslib.Elem(collector.nodes[0]); >+ break; No. As the name of the methods tells it can return a couple of elements, e.g. if you call it with a class selector. > Element.prototype.__defineGetter__("elem", function Element_getElem() { >+ if (!this._elem) { >+ // Wait up to 5 secs for the element to be defined >+ // XXX: should be using a global settings object for the timeout. I would be more in favor of not having a property but a method here, so the timeout can be specified by the caller. Similar to getNode() of the element itself. >+ * Returns true if Element is pointing at a valid node right now, false otherwise >+ * >+ * This will return the instantaneous status. That is, if the node >+ * was once locatable but no longer is locatable, this will return false. >+ * >+ * @name existsNow I wonder if we should call this method 'exists' and rename the other to something which visualizes that it wait for the node. 'existsNow' doesn't really tell me something. >+++ b/tests/functional/testSmoke/testLocationBar.js >+++ b/tests/functional/testSmoke/testWidgets.js Those tests should not be located under functional. Please move them to /lib/tests if we really need them. I don't see a reason for keeping testLocationBar.js. If we want we should rename it to test_browser.js and probably put it into /lib/tests/ui? >+++ b/tests/functional/testToolbar/testBackForwardButtons-new.js Why a new name? >+var head = require("../../../lib/head"); >+var widgets = require("../../../lib/ui/widgets"); >+ >+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/'); Do we wanna go ahead and specify 'root'? so we do not have to use '../../../' all the time? There is a bug on file for Mozmill where we could set the value outside of the test, so if that lands we wouldn't have to change that much code. >+function openPage(page) { >+function clickBackAndVerify(page) { >+function clickForwardAndVerify(page) { Please move all those methods below the test method. That makes it easier to skimming over the test module. >+ if (!element.exists) >+ throw new Error("Could not find element '" + page.id + "' in page '" + page.url + "'"); Why don't you assert? >+function clickBackAndVerify(page) { >+ // This should be waitForPageLoad(), but it currently seems bugged when loading >+ // via back/forward. >+ driver.sleep(500); >+ // browser.waitForPageLoad(); Reason is that we get those pages from the cache. That's why you can't use waitForPageLoad at the moment. That's something which has already been fixed in Mozmill 2. >+/** >+ * Map test functions to litmus tests >+ */ >+// testBackAndForward.meta = {litmusids : [8032]}; >+ We should remove those lines. Those are not valid anymore and only cause confusion. >+// I also rearranged setup/test/teardown in chrono order. Minor, but helps read >+// the flow. Makes more sense to do now that we're one test to a file. I feel it's better to have it directly below setupModule to being able to spot issues for setup / teardown quicker. Also you do not have to scroll down a different number of rows, depending on how long the test is and how many helper functions are following. >+// Generally speaking, I documented where you expect things to be as part of the >+// flow (e.g. this will leave us on PAGES[3]). This helps with following the test. Can we put this file under /templates/? That would be perfect for such an example test. >+var testStopAndReload = function() Please remove the var declaration and put the opening bracket at the same line. We should start consistently to our style guides. Otherwise we wont get what we want to achieve. :) >+ var header = new widgets.Element("id", "header", browser.content.activeTab); >+ assert.ok(!header.existsNow, "Header does not exist"); I would use expect here so we are able to also check for the post-reload state below. >+ // Reload, wait for it to completely load and test again >+ browser.openURL(URL); >+ >+ var header = new widgets.Element("id", "header", browser.content.activeTab); >+ assert.ok(header.exists, "Header exists"); Before reloading the given URL we should call resetTabs() so we have a blank page. I will give r- for now due to the wrong location of some files. I would like to test it myself with the updated patch. Please file new bugs as appropriate for the given comments. If none of those are blocking this landing the next review request will get an r+ after my testing. I hope that's ok for you.

Big reply to a big patch! (In reply to Henrik Skupin (:whimboo) from comment #3) > Comment on attachment 555271 [details] [diff] [review] > Merge in patch form for review > > >+++ b/lib/api/places.js > > Modules which are getting used by tests themselves should not be stored > under API. As discussed those have to land in /lib/ directly. How do we want > to proceed with creating tests for the API? Do we want to create follow-up > bugs for each of the newly integrated modules? I feel we should do this > without getting lazy later. It's a good point. Yeah, should probably be at least filing tasks. Given our timeline, I think deferring unit tests for things used -directly- by production tests is acceptable (the tests themselves exercise it), but we do ultimately want at least basic ones isolated to identify module errors better. > > >+ // Set up an observer so we get notified when remove completes > >+ var expirationSuccessful = false; > >+ let expirationObserver = { ... > > expirationObserver is an instance so shouldn't we move the > expirationSuccessful flag into it and simply name it finished? We can then > access it via expirationObserver.finished. Yes, good idea. I'll do that. > >+ driver.waitFor(function () { > >+ return expirationSuccessful; > >+ }, "History has been removed"); > > Can we ensure that this task will always complete under 5s which is the > default value here? See restoreDefaultBookmarks where we already have a > longer timeout specified. It hasn't raised a timeout error yet. I figured if it started doing so, we'd extend it. I think that function scales based on number of history entries, which for us is typically not-many. What's a value you think we should pick now, based on your experience? > > >+ var importSuccessful = false; > >+ var importObserver = { > > Same as above. Sure, no prob. > > Why do you remove the listener from the innerWindow? It should still exist > so we can catch cases when the ChromeWindow gets closed by other code than > us. Otherwise we will not correctly destroy e.g. a running > defaultModalDialog handler as long as our wrapper instance exists. Not sure what you mean here. As far as I know, all I did was change this.innerWindow.addEventListener() to this.addEventListener(). That -does- use innerWindow.addEventListener(), if you look at the implementation, so it's functionally equivalent. Doing it that way allows us to make the function have better errors, do logging, whatever in the future. If you mean something else, please clarify and give me a code snippet to orient me. > > > /** > >+ * Press a key for the entire window > >+ */ > > We should try to document things on the fly when we create the code. If you > wanna do this later please file a bug. I'll add the docs. That was my bad, I meant to document every public function. > > >+ChromeWindowWrapper.prototype.keypress = > >+function ChromeWindowWrapper_keypress(aKeyCode, aModifiers) { > >+ aModifiers = aModifiers || {}; > >+ return this._controller.keypress(null, aKeyCode, aModifiers); > > In this case you should specify this.innerWindow instead of null. Otherwise > we will fail in Mozmill 2 where null currently doesn't work. Also it makes > it more understandable to know where the event is getting sent to. Sure, no problem. Thanks for letting me know the details on that. > > >+ChromeWindowWrapper.prototype.addEventListener = > >+function ChromeWindowWrapper_addEventListener(aType, aListener, aUseCapture, > >+ aWantsUntrusted) { > >+ this.innerWindow.addEventListener(aType, aListener, aUseCapture, aWantsUntrusted); > >+} > > I think that we still leak memory in those cases. It's nothing new > introduced by your code now, but in our tests we actually never close the > browser window. And for each test we attach a new event handler. We call > destroy() in teardown but the listener never gets freed. Lets do that in a > follow-up bug. It's up to whoever calls this.addEventListener to try..finally it. It's just like the innerWindow one, just exposed outwards, so it's not really a question of the code you quoted. If you're saying the place in ChromeWindowWrapper where I changed the addEventListener() that I mentioned above leaks, then yeah, let's take a look at that. Anything you're adding across a test has to get removed somewhere. But in that case, you know the code better. Any chance you could file that bug? > > >+++ b/lib/dtds.js > > Even tests wouldn't use this module I'm ok in leaving it in /lib/. There can > be cases and code in here is kinda specific similar to the other shared > modules. Well, they might. If they're issuing a shortcut keypress because they're specifically testing that, they might be accessing the DTDs to know which one to issue. Think that's why I actually left it in lib. That's part of our Monday conversation as to what level of operation to expose to the tests though. After that, I'll know how to proceed with this pathing. > > >+const GLOBAL_DTD = "chrome://global/locale/global.dtd"; > >+const BROWSER_DTD = "chrome://browser/locale/browser.dtd"; > >+const TAB_BROWSER_DTD = "chrome://browser/locale/tabbrowser.dtd"; > > Do we really need the _DTD suffix? All constants in this file will be about > DTDs so I don't think it's necessary. Yes, _DTD means a specific DTD resource, as opposed to... > > >+const TAB_BROWSER = [BROWSER_DTD, > >+ TAB_BROWSER_DTD, > >+ GLOBAL_DTD]; > > ... and you don't use it here for TAB_BROWSER. So lets get rid of those? ...which is not a DTD file, it's the array that gets exported outwards. Note that the individual files aren't exported. I didn't give it a suffix because it'd already be: dtds.TAB_BROWSER ...to the code using it, which I thought read well as it is. This'll all be more obvious when there's more than one. :) > >+ //places.restoreDefaultBookmarks(); > >+ //places.removeAllHistory(); > > Why are those methods commented out? Calling those methods per default could > be risky, especially for restart tests which check the history after a > restart. I wonder if setup() should better get an JSON object as second > parameter, so that we can let the test specify as much as possible flags and > don't have to call out each of the parameters. Commented out from when I was debugging the last problem I had with modals. It was a mistake to leave them commented. I don't really want to go to a spec object until we have to. I find them more complex to deal with, and am not sure I want to have that many flags changing test behavior. The idea is to have almost everything behave the same. Let's just do three skip flags for now: bookmarks, history, flags. Any more than that, we go to JSON. It'll only hit a few tests so I'm not worried about the time to refactor it. Is that OK? > > > function teardown(aModule) { > Same here as above. Should be selective by the test, so we don't shoot > ourselves in the knee with restart tests. Sure. Whatever we decide above, we'll do here too. > > >+++ b/lib/services.js > >+ * Service to provide import and export of bookmarks in the Netscape Bookmarks HTML format. > >+ * > >+ * @see <a href="http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsIPlacesImportExportService.idl">nsIPlacesImportExportService</a> > >+ */ > >+XPCOMUtils.defineLazyGetter(services, "importexport", function () { > >+ return Cc["@mozilla.org/browser/places/import-export-service;1"]. > >+ getService(Ci.nsIPlacesImportExportService); > > For non-standard service names we should be more explicit. Otherwise you can > not distinct between different components, e.g. JSON import/export. We could > call it placesImportExport similar to the interface name. Sure, no prob. Keep helping call those out--takes knowing platform to have context to make that decision, and I don't know it well enough yet. > > >+++ b/lib/tests/test_modalDialogDefaultHandler.js > >- head.setup(module); > >+ // Don't reset the tabs--this test doesn't succeed with the > >+ // Clear Recent History modal dialog that results if you do. > >+ // XXX: why not? > >+ head.setup(module, false); > > Please file that as a new bug and add the number in here. I can investigate > that. Promise I will as soon as the code lands (same with any other bugs we do). I don't like having bugs on code that's not in the repo yet. I'll post bug numbers with the landing message, so you can doublecheck then. > >+BrowserWindow.prototype.closeTab = > >+function Browser_closeTab() { > >+ // Turn tab animation off for closure > >+ const PREF_TABS_ANIMATE = "browser.tabs.animate"; > >+ prefs.setPref("boolean", PREF_TABS_ANIMATE, false); > [..] > >+ finally { > >+ prefs.clearUserPref(PREF_TABS_ANIMATE); > >+ } > > You don't have to do this now but please file a bug so we safe-off the > current value of the preference and don't reset it to default afterward but > simply restore the value. Good point. Is there any way for me to check whether it's a user-value when I save it off? I'd rather clear if it was cleared, replace user value if it was user value, rather than always leave it as a user value that's same as the cleared default. Doubt it'd change anything functionally, but it's the Right Way to do it. > > >+ // Hit the shortcut key for closing tabs > >+ var cmdKey = l10n.getEntity(dtds.TAB_BROWSER, "closeCmd.key"); > >+ this.keypress(cmdKey, {accelKey: true}); > > As talked on IRC we want to make use of the menu per default. Lets get this > done in a followup bug. Will do. > > >+++ b/lib/ui/widgets.js > >+ var collector = this._getCollector(); > >+ collector.queryAnonymousNodes(key, val); > >+ > >+ // No need to check for multiple; node collector only returns 1 or 0 for anon. > >+ > >+ if (collector.nodes.length = 1) > >+ elem = new elementslib.Elem(collector.nodes[0]); > >+ break; > > No. As the name of the methods tells it can return a couple of elements, > e.g. if you call it with a class selector. queryAnonymousNodes : function nodeCollector_queryAnonymousNodes(aAttribute, aValue) { var node = this._document.getAnonymousElementByAttribute(this._root, aAttribute, aValue); this.nodes = node ? [node] : [ ]; return this; The nodes array can only contain one node or zero nodes, per the ternary operator in your code right above the chained return. If I'm misreading this, I'll need you to explain how and what I should be doing instead. > > > Element.prototype.__defineGetter__("elem", function Element_getElem() { > >+ if (!this._elem) { > >+ // Wait up to 5 secs for the element to be defined > >+ // XXX: should be using a global settings object for the timeout. > > I would be more in favor of not having a property but a method here, so the > timeout can be specified by the caller. Similar to getNode() of the element > itself. Won't work. elem is the thing -every other element function and property- goes through (except existsNow) so that they all honor the wait. In order to do what you're saying, you'd have to expose the parameter on every other element function and have no properties at all, since they can't have parameters. To make this configurable, you have to do something like what I XXX'd (and probably should file a bug for here) and create global data somewhere that gets referenced by every operation. I prefer a singleton object for this; the reference to it can be injected during head. So then you get something like: options.waitTime = 10000; // default wait 10 secs myButton.click(...); options.waitTime = 0; // default wait disabled myButton.click(...); options.waitTime = 5000; // returned to default In real life, you'd probably save it off and restore it, but you get my point. You have to use the same type of thing for anything that changes basic harness/map behavior because otherwise there's just too many places you'd have to expose parameters. Can also have the singleton read "starting defaults" from a cfg file so that it can be changed test-run-wide. > > >+ * Returns true if Element is pointing at a valid node right now, false otherwise > >+ * > >+ * This will return the instantaneous status. That is, if the node > >+ * was once locatable but no longer is locatable, this will return false. > >+ * > >+ * @name existsNow > > I wonder if we should call this method 'exists' and rename the other to > something which visualizes that it wait for the node. 'existsNow' doesn't > really tell me something. I'd really like the default functions/properties to all honor the wait. So assuming people use exists in the default case, I'd like that to be the one that waits. Re: the other, I considered existsNoWait, which sucked, and existsImmediately, which was too long. So I settled on "exists right now" as the concept, shortened to existsNow. If you still hate it, how about exists becomes a function, with a parameter that lets you redefine the timeout from elem's default. It's the one place I'd be fairly OK with that approach, since I can see wanting variable timing on this operation. fooButton.exists(); // according to implicit waitFor fooButton.exists(0); // right now fooButton.exists(15000); // like a waitFor exists with 15 secs as timeout > > >+++ b/tests/functional/testSmoke/testLocationBar.js > >+++ b/tests/functional/testSmoke/testWidgets.js > > Those tests should not be located under functional. Please move them to > /lib/tests if we really need them. I don't see a reason for keeping > testLocationBar.js. If we want we should rename it to test_browser.js and > probably put it into /lib/tests/ui? Sure, I'll move them both into /lib/tests/ui and rename. > > >+++ b/tests/functional/testToolbar/testBackForwardButtons-new.js > > Why a new name? Because it's a prototype to show you how I'd write the test, not the actual port (until/unless you end up liking it), so I didn't remove the other testBackForwardButtons. It'll go away once we decide on what we both like as far as test standards go; testBackForwardsButtons will be the final code. > > >+var head = require("../../../lib/head"); > >+var widgets = require("../../../lib/ui/widgets"); > >+ > >+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/'); > > Do we wanna go ahead and specify 'root'? so we do not have to use > '../../../' all the time? There is a bug on file for Mozmill where we could > set the value outside of the test, so if that lands we wouldn't have to > change that much code. Yeah, I'm fine with that. Wish there was a way to specify it in a whole directory of tests short of creating a one-off module for each directory and having to import that. For now, I'll move it into a root const within the test. > > >+function openPage(page) { > >+function clickBackAndVerify(page) { > >+function clickForwardAndVerify(page) { > > Please move all those methods below the test method. That makes it easier to > skimming over the test module. You and I have far different feelings on that one. I prefer helpers above, main below. That's mostly because it can be done in any programming language (many require dependencies declared first) so most of us old-skoolers are used to it. ;) Still, I think I'm OK with this. Let me give it a shot and see if I hate it. > > >+ if (!element.exists) > >+ throw new Error("Could not find element '" + page.id + "' in page '" + page.url + "'"); > > Why don't you assert? Because it's not a test function, it's a setup function. This is the conversation you and I need to have. The test is -only- the state change we're testing. Everything else is fixture (i.e. setting up the test). Only tests assert. Fixture setups do not assert, they throw errors. Obviously, you end up loosening that for tests that move around and check a bunch of things, but this test tests the back/forward buttons, not opening pages, so that's all that lives in test. There'll be another test that tests opening pages, and it'll assert. > > >+function clickBackAndVerify(page) { > >+ // This should be waitForPageLoad(), but it currently seems bugged when loading > >+ // via back/forward. > >+ driver.sleep(500); > >+ // browser.waitForPageLoad(); > > Reason is that we get those pages from the cache. That's why you can't use > waitForPageLoad at the moment. That's something which has already been fixed > in Mozmill 2. Good to know. That was pretty confusing. > > >+/** > >+ * Map test functions to litmus tests > >+ */ > >+// testBackAndForward.meta = {litmusids : [8032]}; > >+ > > We should remove those lines. Those are not valid anymore and only cause > confusion. Another conversation you and I need to have is how we can get a link from the test back to Litmus; it may not be this way, but it needs to be some way, and I'm not willing to wait for TCM for it. I don't find the one-way link sufficient to know exactly what version of the test the file was written against and verify it's correct. If there is no way to link, I'm going to want to copy the text of the test into a comment section into the file, since it's the only way to know what the dev was looking at when they wrote it. Consider figuring out a linking solution as preventing having to deal with that. :) > > >+// I also rearranged setup/test/teardown in chrono order. Minor, but helps read > >+// the flow. Makes more sense to do now that we're one test to a file. > > I feel it's better to have it directly below setupModule to being able to > spot issues for setup / teardown quicker. Also you do not have to scroll > down a different number of rows, depending on how long the test is and how > many helper functions are following. Teardown also tears down things that happened during the test that weren't try..finally'ed (and much of the reason you do teardown is to avoid a crapload of try finallies in your test). I need to be able to scan from top to bottom, see everything that was created, and make sure it was removed. By having the test below teardown, you probably ignore things the test did. > > >+// Generally speaking, I documented where you expect things to be as part of the > >+// flow (e.g. this will leave us on PAGES[3]). This helps with following the test. > > Can we put this file under /templates/? That would be perfect for such an > example test. Sure, once we get it in a shape you and I both like it. I'd like to have a few different "canonical" tests in there. > > >+var testStopAndReload = function() > > Please remove the var declaration and put the opening bracket at the same > line. We should start consistently to our style guides. Otherwise we wont > get what we want to achieve. :) Yeah, dunno where the heck that came from. I know better. :P > > >+ var header = new widgets.Element("id", "header", browser.content.activeTab); > >+ assert.ok(!header.existsNow, "Header does not exist"); > > I would use expect here so we are able to also check for the post-reload > state below. If the header didn't exist the first time, doesn't it mean the test is off the rails? But OK, no biggie to me. > > >+ // Reload, wait for it to completely load and test again > >+ browser.openURL(URL); > >+ > >+ var header = new widgets.Element("id", "header", browser.content.activeTab); > >+ assert.ok(header.exists, "Header exists"); > > Before reloading the given URL we should call resetTabs() so we have a blank > page. It's testing the reload button. If I do that, it'll reload the blank page. :) That's the reason I asserted above instead of expecting; we're reloading the page already in browser. > > I will give r- for now due to the wrong location of some files. I would like > to test it myself with the updated patch. Please file new bugs as > appropriate for the given comments. If none of those are blocking this > landing the next review request will get an r+ after my testing. I hope > that's ok for you. Sure. I'm going to go ahead and work on the quickie changes. Please comment back when you get a chance so I can nail down the controversial ones.

re: resetTabs() just above, ignore that. I thought this one tested the reload button, but it doesn't. You're right, and I'll make that change.

Actually, you know, that StopReload test is just plain broken as originally written. As it is now, if the element doesn't exist on the page at all (i.e. your data is screwed up), it'll pass the first part but not the second, which is confusing. If it does exist and stop is broken, it'll fail the first part and pass the second. Either way you get a failure, so yeah we'll investigate, but they're really uninformative failures and not well isolated. IMO, should be: Setup: // Precheck the fixture (page) Load page completely check for element, error out if not there clear cache so that there's load time and the stop has a chance to succeed Test: // Test stop Load page and stop it Check for no element, assert if there That gives you an error in setup if the data or internal httpd is hosed (or if loading pages is broken, but you'll get errors across the harness if that's the case). Then you get a failure in the test if Stop is broken. We can keep the reload part if what we're testing there is that we don't cache the half-loaded version. If that's not what we're testing, seems spurious to me. But it doesn't replace the precheck, either way, would be additional. I think that's exactly what you want, and I'd like to change the test. I'll need to find out how to dump cache programmatically, but we need that elsewhere anyway (should be part of our implicit setup/teardown, in fact...maybe that gets you your JSON object for being flag #4). Any idea how to do that? BTW, this is one of those cases where the litmus version probably has it in the exact order we implemented it, but it's different because it's doing a visual check of the page and it's probably a popular page people already know is a known-good and they know the httpd serving it works. The automation should at least pre-check that the page is good and can load, so that the test is properly isolated.

(In reply to Geo Mealer [:geo] from comment #4) > > >+ driver.waitFor(function () { > > >+ return expirationSuccessful; > > >+ }, "History has been removed"); > > > > Can we ensure that this task will always complete under 5s which is the > > default value here? See restoreDefaultBookmarks where we already have a > > longer timeout specified. > > It hasn't raised a timeout error yet. I figured if it started doing so, we'd > extend it. I think that function scales based on number of history entries, > which for us is typically not-many. > > What's a value you think we should pick now, based on your experience? I would suggest the same value as for restoreDefaultBookmarks for now. With that value we should have a good safety net. > > Why do you remove the listener from the innerWindow? It should still exist > > so we can catch cases when the ChromeWindow gets closed by other code than > > us. Otherwise we will not correctly destroy e.g. a running > > defaultModalDialog handler as long as our wrapper instance exists. > > Not sure what you mean here. As far as I know, all I did was change > this.innerWindow.addEventListener() to this.addEventListener(). > > That -does- use innerWindow.addEventListener(), if you look at the > implementation, so it's functionally equivalent. Doing it that way allows us > to make the function have better errors, do logging, whatever in the future. > > If you mean something else, please clarify and give me a code snippet to > orient me. Oh, that was a bit out-dated comment. I wanted to remove it when I have seen the changes in windows.js. Just missed it. > > In this case you should specify this.innerWindow instead of null. Otherwise > > we will fail in Mozmill 2 where null currently doesn't work. Also it makes > > it more understandable to know where the event is getting sent to. > > Sure, no problem. Thanks for letting me know the details on that. Also see bug 677364, which hopefully gets fixed for Mozmill 2. > > >+ChromeWindowWrapper.prototype.addEventListener = > > >+function ChromeWindowWrapper_addEventListener(aType, aListener, aUseCapture, > > >+ aWantsUntrusted) { > > >+ this.innerWindow.addEventListener(aType, aListener, aUseCapture, aWantsUntrusted); > > >+} > > > > I think that we still leak memory in those cases. It's nothing new > > introduced by your code now, but in our tests we actually never close the > > browser window. And for each test we attach a new event handler. We call > > destroy() in teardown but the listener never gets freed. Lets do that in a > > follow-up bug. > > It's up to whoever calls this.addEventListener to try..finally it. It's just > like the innerWindow one, just exposed outwards, so it's not really a > question of the code you quoted. > > If you're saying the place in ChromeWindowWrapper where I changed the > addEventListener() that I mentioned above leaks, then yeah, let's take a > look at that. Anything you're adding across a test has to get removed > somewhere. So the thing is that for each new test we add a new event handler for unload. Because we do NOT close the actual browser window after a test, the attached event listener will not be removed but we are adding more and more, which in parallel will call the destroy method at one point. We should simply remove the event listener for unload in destroy(). > > Do we really need the _DTD suffix? All constants in this file will be about > > DTDs so I don't think it's necessary. > > Yes, _DTD means a specific DTD resource, as opposed to... In this case could we use it as prefix then? It's common for all those constants and the high-level specifier. So having it as prefix makes more sense. What do you think? > Let's just do three skip flags for now: bookmarks, history, flags. Any more > than that, we go to JSON. It'll only hit a few tests so I'm not worried > about the time to refactor it. > > Is that OK? Sure. > > > function teardown(aModule) { > > > Same here as above. Should be selective by the test, so we don't shoot > > ourselves in the knee with restart tests. > > Sure. Whatever we decide above, we'll do here too. Good. > > >+BrowserWindow.prototype.closeTab = > > >+function Browser_closeTab() { > > >+ // Turn tab animation off for closure > > >+ const PREF_TABS_ANIMATE = "browser.tabs.animate"; > > >+ prefs.setPref("boolean", PREF_TABS_ANIMATE, false); > > [..] > > >+ finally { > > >+ prefs.clearUserPref(PREF_TABS_ANIMATE); > > >+ } > > > > You don't have to do this now but please file a bug so we safe-off the > > current value of the preference and don't reset it to default afterward but > > simply restore the value. > > Good point. Is there any way for me to check whether it's a user-value when > I save it off? I'd rather clear if it was cleared, replace user value if it > was user value, rather than always leave it as a user value that's same as > the cleared default. > > Doubt it'd change anything functionally, but it's the Right Way to do it. Lets move this off to another bug because it seems we have to think about it a bit more. > > No. As the name of the methods tells it can return a couple of elements, > > e.g. if you call it with a class selector. > > queryAnonymousNodes : function nodeCollector_queryAnonymousNodes(aAttribute, > aValue) { > var node = this._document.getAnonymousElementByAttribute(this._root, > aAttribute, > aValue); > this.nodes = node ? [node] : [ ]; > return this; > > The nodes array can only contain one node or zero nodes, per the ternary > operator in your code right above the chained return. > > If I'm misreading this, I'll need you to explain how and what I should be > doing instead. Wow, the function name is totally mis-leading then and has to be updated to queryAnonymousNode. > To make this configurable, you have to do something like what I XXX'd (and > probably should file a bug for here) and create global data somewhere that > gets referenced by every operation. I prefer a singleton object for this; > the reference to it can be injected during head. Hm, but even then the timeouts will not be accessible by the sub modules and you will have to pass those down to the function calls or by using persisted. > If you still hate it, how about exists becomes a function, with a parameter > that lets you redefine the timeout from elem's default. It's the one place > I'd be fairly OK with that approach, since I can see wanting variable timing > on this operation. > > fooButton.exists(); // according to implicit waitFor > fooButton.exists(0); // right now > fooButton.exists(15000); // like a waitFor exists with 15 secs as timeout That would be perfect IMO and absolutely variable in its usage. > > >+var head = require("../../../lib/head"); > > >+var widgets = require("../../../lib/ui/widgets"); > > >+ > > >+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/'); > > > > Do we wanna go ahead and specify 'root'? so we do not have to use > > '../../../' all the time? There is a bug on file for Mozmill where we could > > set the value outside of the test, so if that lands we wouldn't have to > > change that much code. > > Yeah, I'm fine with that. Wish there was a way to specify it in a whole > directory of tests short of creating a one-off module for each directory and > having to import that. > > For now, I'll move it into a root const within the test. I lacked the bug number for this. Please see bug 641682 which I now have reopened. > You and I have far different feelings on that one. I prefer helpers above, > main below. That's mostly because it can be done in any programming language > (many require dependencies declared first) so most of us old-skoolers are > used to it. ;) Good point. and taken. > > >+/** > > >+ * Map test functions to litmus tests > > >+ */ > > >+// testBackAndForward.meta = {litmusids : [8032]}; > > >+ > > > > We should remove those lines. Those are not valid anymore and only cause > > confusion. > > Another conversation you and I need to have is how we can get a link from > the test back to Litmus; it may not be this way, but it needs to be some > way, and I'm not willing to wait for TCM for it. > > I don't find the one-way link sufficient to know exactly what version of the > test the file was written against and verify it's correct. > > If there is no way to link, I'm going to want to copy the text of the test > into a comment section into the file, since it's the only way to know what > the dev was looking at when they wrote it. Consider figuring out a linking > solution as preventing having to deal with that. :) There is no way. We could now at least add links to tests of the Aurora branch on Litmus. Those links will never be out of date. Beside that a date could be used to specify when the test has been created or overhauled? > > >+ var header = new widgets.Element("id", "header", browser.content.activeTab); > > >+ assert.ok(!header.existsNow, "Header does not exist"); > > > > I would use expect here so we are able to also check for the post-reload > > state below. > > If the header didn't exist the first time, doesn't it mean the test is off > the rails? But OK, no biggie to me. There could be other failures beside this one. > > Before reloading the given URL we should call resetTabs() so we have a blank > > page. > > It's testing the reload button. If I do that, it'll reload the blank page. > :) That's the reason I asserted above instead of expecting; we're reloading > the page already in browser. Crap. Forget those comments then.

Got put back today prepping for chemspill. I'm pasting in the list of what I'm working on, will get it merged as soon as I've done the ones that aren't "Bug for". Despite the length, it's mostly minor touchup. * lib/api/places.js -> lib/places.js * import/expirationSuccessful flag into objects in places.js * expirationSuccessful timeout hardcode to 5000 * Documentation for window.keypress * keypress: null -> innerWindow * Add removeEventListener call in destroy() for ChromeWindowWrapper * _DTD suffix -> DTD_ prefix * Uncomment reset methods in head.js * Add flags to select methods in setup * Add flags to select methods in teardown * services.importexport -> placesImportExport * Bug for modal unit test not working if tabs are cleared * Bug for closeTab preference saving off, need to know how to check if user and clear if so. * Bug for nodeCollector.queryAnonymousNodes namechange. * Bug to add global object for elem timeout and other data * exists: property->function w/ timeout parameter * remove existsNow * move testBackForward (new) helper functions below mains. * Remove litmus IDs for now, need followup as to linking to Aurora or what. * testStopAndReload() -> correct function style * testStopAndReload, assert->expect for header check * resetTabs() before reloading in testStopAndReload

This is requested as feedback because I've already pushed the merge for sake of expediency to unblock you tomorrow. If there's something you feel needs changing to unblock you, please just change it and push. I intentionally didn't collapse commits because I wanted my checkin history preserved in case of problems. You can see commits here; it's everything north of your last one: https://github.com/geoelectric/mozmill-api-refactor/commits/master The attached patch is what was merged. There was one additional change I forgot to regenerate the patch after: I took out the Litmus ID comments from the bottom of the other two tests, not just the one mentioned in the bug. Here's the list of fixes, as well as the TODOs for filing bugs. These will be filed, but not today--I need to get back to chemspill now. FIXED * lib/api/places.js -> lib/places.js FIXED * import/expirationSuccessful flag into objects in places.js FIXED * expirationSuccessful timeout hardcode to 10000 FIXED * Documentation for window.keypress WONTFIX * keypress: null -> innerWindow: mozmill tries to call getNode() on whatever's sent in if it's not null. See error below. mozmill 2.0 will be broken regardless, as it calls target.keypress() ERROR | Test Failure: {"exception": {"stack": "([object Proxy],\"W\",[object Proxy])@resource://mozmill/modules/controller.js:350\n", "message": "aTarget.getNode is not a function", "fileName": "resource://mozmill/modules/controller.js", "name": "TypeError", "lineNumber": 350}} FIXED * Add removeEventListener call in destroy() for ChromeWindowWrapper Changed constructor to use destroy() directly as the callback to addEventListener('unload'). Had to be a named function at the class level to be able to remove it, and I realized I was writing a function that did nothing but call destroy() so calling destroy() directly was the right answer. FIXED * Uncomment reset methods in head.js FIXED * Add flags to select methods in setup FIXED * Add flags to select methods in teardown FIXED * services.importexport -> placesImportExport FIXED * exists: property->function w/ timeout parameter FIXED * remove existsNow FIXED * move testBackForward (new) helper functions below mains. FIXED * Remove litmus IDs for now FIXED * testStopAndReload() -> correct function style FIXED * testStopAndReload, assert->expect for header check FIXED * resetTabs() before reloading in testStopAndReload TODO * Bug for modal unit test not working TODO * Bug for closeTab preference saving off, need to know how to check if user and clear if so. TODO * Bug for nodeCollector.queryAnonymousNodes namechange. TODO * Bug to add global object for elem timeout and other data TODO * Bug/Discussion re: linking tests to Litmus TODO * Bug against testStopAndReload (refactor and probably current codebase too), hitting stop button immediately doesn't actually stop page from fully loading in that test (tried it manually). testStopAndReload is succeeding only because it checks the element before the page is fully loaded. Probably should hardcode a small sleep (waitFor wouldn't be appropriate here, I don't think) in front of the check so that this situation throws a failure.

Comment on attachment 558202 [details] [diff] [review] Full merge, including followup changes Hard to interdiff because files have been changed and Bugzilla can't handle that. :( But a quick glance over the code doesn't show me any obvious things except those you already have on your list to file as bugs. So it's fine. Thanks for the merge!

Geo, will you be able to file all the follow-up bug by this week or early next week. Would be necessary so we can gain the maximum efforts next week. Thanks.

This one's been done for awhile.