User-Agent: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 Initial landing of the code of the fidesfit-client tests Reproducible: Always

If the patch is ready for review please ask for it on the attachment.

Comment on attachment 449646 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension Trying to ask for review

Henrik, have I managed to properly ask the patch for review?

Comment on attachment 449646 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension I have got a better patch coming, please ignore this one.

Comment on attachment 450081 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension >+ get statusbar_panel() { >+ return new elementslib.ID(this._controller.window.document, >+ 'fidesfit-statusbar-panel'); >+ }, I would advise you to also have a getElement function which can be used to have all that code within one function. >diff -r 1844482c24e5 addons/fidesfit-client@fidesfit.org/shared-modules/testModalDialogAPI.js Seeing this code duplicated makes me sad. But looks like there is no other way around. Or wait, could you try to reference the global shared api's from within your fidesfit client config api? Given that it should be possible that we do not have to duplicate all that code. >+ // Making the gBrowser available for all tests >+ var window = Components.classes['@mozilla.org/appshell/window-mediator;1'] >+ .getService(Components.interfaces.nsIWindowMediator) >+ .getMostRecentWindow('navigator:browser'); nit: You can use Cc and Ci as shorter names. Also place the dot at the end of the line and align the left with Cc. It applies to different areas. >+ controller.click(new elementslib.ID(controller.window.document, >+ 'fidesfit-statusbar-suggest')); From our experience its always useful to move all of the element creation code out into a shared module. It's something you have already started. So why not finishing it? I haven't checked the code that deeply yet, only tried to get an overview for the first stage. What I have also seen is that you aren't using any comments. It's probably a good idea to add at least the ones for your functions. All together please try to use our shared modules via the way I have proposed. I really would like to see that method which should be possible.

(In reply to comment #7) > > Or wait, could you try to reference the global shared api's from within > your fidesfit client config api? > Please could you be more specific? Could you give an example of what you are suggesting?

Comment on attachment 450081 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension >+++ b/addons/fidesfit-client@fidesfit.org/shared-modules/testFidesfitHelper.js Wed Jun 09 12:36:39 [...] >+ >+const MODULE_NAME = 'FidesfitHelper'; It was probably not the config but the FidesfitHelper module. Add right below this module name declaration, the code to include our shared modules. Then you can access those function via the FidesfitHelper module.

I have acted upon all your requests, except the point related to the shared modules to which I will reply in another comment.

(In reply to comment #9) > (From update of attachment 450081 [details] [diff] [review]) > >+++ b/addons/fidesfit-client@fidesfit.org/shared-modules/testFidesfitHelper.js Wed Jun 09 12:36:39 > [...] > >+ > >+const MODULE_NAME = 'FidesfitHelper'; > > It was probably not the config but the FidesfitHelper module. Add right below > this module name declaration, the code to include our shared modules. Then you > can access those function via the FidesfitHelper module. I gave it a try with no success, having error messages such as the following for my different tries: ERROR - Test Failure: {u'exception': {u'message': u'FidesfitHelper.preferences is undefined' ERROR - Test Failure: {u'exception': {u'message': u'FidesfitHelper.PrefsAPI is undefined'

Comment on attachment 451064 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension I would like to wait with the review until we have the other issue resolved. So, for shared module inclusion I have an example for you. It's the way how we are doing it for our own tests: http://hg.mozilla.org/qa/mozmill-tests/file/507d3aa49e8a/shared-modules/testPrivateBrowsingAPI.js

This new patch has the shared module inclusion in the FidesfitHelper as you requested.

Comment on attachment 451288 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension Sorry for the delay but I had to finish some important stuff the last days. >+[download] >+linux=https://download.fidesfit.org/browser-extensions/firefox/fidesfit-client/fidesfit-client-latest.xpi >+mac=https://download.fidesfit.org/browser-extensions/firefox/fidesfit-client/fidesfit-client-latest.xpi >+win=https://download.fidesfit.org/browser-extensions/firefox/fidesfit-client/fidesfit-client-latest.xpi Is the latest version of the extension also compatible with Minefield? >+// Include necessary modules >+var RELATIVE_ROOT = '.'; >+var MODULE_REQUIRES = ['ModalDialogAPI', 'PrefsAPI', 'UtilsAPI']; You are still using your own shared modules. Have you forgotten to qrefresh the patch? >+ enableDebugLog: function fidesfitHelper_setDebugLog() { >+ this._PrefsAPI.preferences.setPref('extensions.fidesfit-client.debug.level', 1); >+ }, Can we get this disabled per default please. It will only logged to the console but not send to our reporting server. Please also update test_configure.js. On OS X there is no close button. Given that the complete test-run hangs.

I have a new patch almost ready with improvements: no more debug enabled by default (sorry), more element getters in the FidesfitHelper, more relevant tests and more comments. But : 1. I haven't found how to close the prefwindow involved in test_configure.js and that you are mentioning without using the close button. Could you provide me with suggestions regarding this point please? 2. Yes the FidesfitHelper is still using the Mozmill shared modules. I thought that was what you were suggesting me. FidesfitHelper is acting as a wrapper to access the shared modules, and thus the shared modules specific code is only present in one file: that is in FidesfitHelper. Could please elaborate what should be done here instead? At least modification regarding the use of shared modules are only to be done in one place now :-)

(In reply to comment #15) > 1. I haven't found how to close the prefwindow involved in test_configure.js > and that you are mentioning without using the close button. Could you provide > me with suggestions regarding this point please? Changes should be applied immediately on OS X. So synthesizing VK_ESCAPE should do the trick. See our preferencesDialog_close function: http://hg.mozilla.org/qa/mozmill-tests/file/ab45c39ec912/shared-modules/testPrefsAPI.js#l122 > 2. Yes the FidesfitHelper is still using the Mozmill shared modules. I thought > that was what you were suggesting me. FidesfitHelper is acting as a wrapper to > access the shared modules, and thus the shared modules specific code is only > present in one file: that is in FidesfitHelper. Could please elaborate what > should be done here instead? At least modification regarding the use of shared > modules are only to be done in one place now :-) You shouldn't fork those modules for your add-on. Instead change the RELATIVE_ROOT so it will point to the global shared-modules folder.

(In reply to comment #16) > You shouldn't fork those modules for your add-on. Instead change the > RELATIVE_ROOT so it will point to the global shared-modules folder. Ok, I think I got it. My job is now to make this RELATIVE_ROOT works for the mozmill-tests repo as well as for the Fidesfit-client repo. Thanks. I'll post a new patch ASAP.

After hours of test: that doesn't work with Mozmill. Each test*.js file needs to access the testFidesfitHelper.js module which is in the extension shared-modules directory, and thus can't access at the same time (only one declaration of RELATIVE_ROOT is taken into account by mozmill) the RELATIVE_ROOT of the mozmill shared-modules. That leaves just one other possibility: make the testFidesfitHelper.js module have a RELATIVE_ROOT pointing to the mozmill shared-modules ("../../shared-modules/") instead of the current "." path. And that doesn't work with the error below. As a side note, and as of now, the toolbar@google.com extension is not in this case, the toolbar@google.com only use its shared-modules. ERROR - Test Failure: {'exception': {'message': 'collector.getModule("UtilsAPI") is undefined', 'lineNumber': 203, 'stack': 'fidesfitHelper_assertElementVisible([object Object],true)@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js -> file:///home/firefox/fidesfit-client@fidesfit.org/test/mozmill/shared-modules/testFidesfitHelper.js:203\n()@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js -> file:///home/firefox/fidesfit-client@fidesfit.org/test/mozmill/tests/test_configure.js:60\n((function () {var fidesfit_helper = new (FidesfitHelper.FidesfitHelper)(controller);var statusbar_panel = fidesfit_helper.getElement({type: "statusbar-panel"});controller.assertNode(statusbar_panel);controller.rightClick(statusbar_panel);fidesfit_helper.assertElementVisible(fidesfit_helper.getElement({type: "statusbar-menu"}), true);fidesfit_helper.setModalDialogCbHandler(configureWindowCallbackHandler);controller.waitThenClick(fidesfit_helper.getElement({type: "statusbar-config-menuitem"}));}))@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js:468\n([object Object])@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js:520\n([object Object])@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js:562\n("/home/firefox/fidesfit-client@fidesfit.org/test/mozmill/tests/test_configure.js")@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js:420\n("/home/firefox/fidesfit-client@fidesfit.org/test/mozmill/tests/test_configure.js")@file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js:574\n((function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Array])@file:///tmp/tmpogT5hx.mozrunner/extensions/jsbridge@mozilla.com/resource/modules/server.js:164\n("941a55b4-8513-11df-a1bf-0016e686b102",(function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Array])@file:///tmp/tmpogT5hx.mozrunner/extensions/jsbridge@mozilla.com/resource/modules/server.js:168\n@file:///tmp/tmpogT5hx.mozrunner/extensions/jsbridge@mozilla.com/resource/modules/server.js:244\n', 'fileName': 'file:///tmp/tmpogT5hx.mozrunner/extensions/mozmill@mozilla.com/resource/modules/frame.js -> file:///home/firefox/fidesfit-client@fidesfit.org/test/mozmill/shared-modules/testFidesfitHelper.js'}} From what I've read from the frame.js code, that could come from the loading/harvesting of modules code, that may never have been designed for such a case. Debugging this module-loading part of the mozmill code doesn't sound that difficult but will take some time, time that I haven't now unfortunately.

(In reply to comment #14) > > Is the latest version of the extension also compatible with Minefield? > I have added Minefield to the targeted platforms. 6 tests out of 19 don't pass at the moment :-\

Would be great if you could create a simple testcase with the multiple module inclusion. It has to work, so I wonder what's wrong here. Also please mention the problem in the mozmill-dev list so we can try to solve it. (In reply to comment #19) > I have added Minefield to the targeted platforms. 6 tests out of 19 don't pass > at the moment :-\ First we should try to have it ready for 1.9.2 and 1.9.1. Then it makes sense to work on the default integration.

This patch contains tests passing with Firefox 3.5 and Firefox 3.6. This patch includes: - the modification hopefully adding support for Mac OS X. - disabling of debugs This patch doesn't include the fix for the non-duplication of the mozmill shared modules, since I've not been able to do it and maybe mozmill has a problem with that. I'll address that by providing a separate test case showing the problem. In the meanwhile I would appreciate if you could review this patch further and validate that this patch passes on Mac OS X alright. Thanks :-)

This patch is in sync with the latest version of the fidesfit-client-latest.xpi file. This patch also features a monkey-patch/mockup of a method in the net.jsm module to fake requests to the Fidesfit server, so it's possible to test more thoroughly. This monkey-patch also highlight the fact that mozmills leaks JSM modules from one tests file to another, but it's dealt with in this patch. I'll address this what-seems-a-bug-in-mozmill somewhere else. I was just mentioning it to ease the review.

This patch is in sync with the latest version of the fidesfit-client-latest.xpi file. This patch features more assertions, especially for testing the SQLite interactions, and a cleaner integration of the NetMockup. Henrik, I'm very eager to have you test the or Mac OS X compatibility.

Comment on attachment 458069 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension I have tested with Namoroka on OS X again and I get 4 test failures. You will find those below with more or less information: ERROR - Test Failure: {u'exception': {}} Test Failed : testBlacklistUniqueness in /private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpLfEVs6.mozmill-tests/addons/fidesfit-client@fidesfit.org/tests/test_module_decision.js ERROR - Test Failure: {u'exception': {}} Test Failed : testBlacklistHost in /private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpLfEVs6.mozmill-tests/addons/fidesfit-client@fidesfit.org/tests/test_module_decision.js ERROR - Test Failure: {u'exception': {}} Test Failed : testBlacklistDomain in /private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpLfEVs6.mozmill-tests/addons/fidesfit-client@fidesfit.org/tests/test_module_decision.js ERROR - Test Failure: {u'exception': {u'message': u'Controller.assertProperty(ID: fidesfit-statusbar-activation) : chrome://fidesfit-client/skin/activation-13-disabled.png == chrome://fidesfit-client/skin/activation-13.png', u'lineNumber': 784, u'stack': u'Error("Controller.assertProperty(ID: fidesfit-statusbar-activation) : chrome://fidesfit-client/skin/activation-13-disabled.png == chrome://fidesfit-client/skin/activation-13.png")Test Failed : testActivationImageClick in /private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpLfEVs6.mozmill-tests/addons/fidesfit-client@fidesfit.org/tests/test_statusbar.js Passed 30 :: Failed 4 :: Skipped 0 Further you should really get rid of the sleep(500) calls over all the files. If you need to wait for something please use waitForEval (or waitFor in the upcoming Mozmill 1.4.2 version), listen to events, or create observers.

Those tests all pass on my system: Debian GNU/Linux 5.0.5 (lenny) + Firefox 3.6.8 + MozMill 1.5.0 This patch is against a fixed version of the extension .XPI (fidesfit-client-0.17.7.xpi instead of fidesfit-client-latest.xpi) which is the kind of stability we all seek. And finally I have started to replace the sleep calls with waitForEval calls. It's just the beginning of this task, since it takes time to find the right condition each time. I have tried to use waitFor but found it harder to use than waitForEval so I thought that I was on the wrong track.

(In reply to comment #25) > Those tests all pass on my system: Debian GNU/Linux 5.0.5 (lenny) + Firefox > 3.6.8 + MozMill 1.5.0 Is it also supported for 4.0 yet? > And finally I have started to replace the sleep calls with waitForEval calls. > It's just the beginning of this task, since it takes time to find the right > condition each time. I have tried to use waitFor but found it harder to use > than waitForEval so I thought that I was on the wrong track. It's kinda hard for me to check the differences between the different patches attached. It's getting bigger and bigger. I would suggest that we do not make major design updates to the tests for the upcoming reviews but we should try to eliminate existing failures. Other stuff like using waitFor (which I would suggest in favor of waitForEval) should be moved to it's own bug. We should really try to get this patch landed. I will give it a shot on my system today.

(In reply to comment #26) > (In reply to comment #25) > > Those tests all pass on my system: Debian GNU/Linux 5.0.5 (lenny) + Firefox > > 3.6.8 + MozMill 1.5.0 > > Is it also supported for 4.0 yet? > Not yet. > > And finally I have started to replace the sleep calls with waitForEval calls. > > It's just the beginning of this task, since it takes time to find the right > > condition each time. I have tried to use waitFor but found it harder to use > > than waitForEval so I thought that I was on the wrong track. > > It's kinda hard for me to check the differences between the different patches > attached. It's getting bigger and bigger. > I'm sorry for that. I had to add new functionalities ask by the company. > I would suggest that we do not make > major design updates to the tests for the upcoming reviews but we should try to > eliminate existing failures. OK > Other stuff like using waitFor (which I would > suggest in favor of waitForEval) should be moved to it's own bug. I'll do that as a background job. Is a ticket always needed to land a patch? > really try to get this patch landed. I will give it a shot on my system today. Thanks

Comment on attachment 469412 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension Sorry that I have to decline the patch again but running those tests with the latest 3.6.8 release or a Namoroka build always gives me 15 failures. See the following report: http://brasstacks.mozilla.com/couchdb/mozmill-crowd/_design/dashboard/_show/report/d37f8f06049accd405a3f2b55df5294d All details see: http://brasstacks.mozilla.com/couchdb/mozmill-crowd/d37f8f06049accd405a3f2b55df5294d Also please remove the code which creates the fidesfit-client.debug file in the users home directory. We do not want to touch any other directories of the users system.

(In reply to comment #27) > I'll do that as a background job. Is a ticket always needed to land a patch? The review process is needed. So yes, always file a new bug and mark it blocking the tracking bug for your companies add-on.

(In reply to comment #28) > Comment on attachment 469412 [details] [diff] [review] > Patch against mozmill-tests to add addon tests for fidesfit-client extension > > Sorry that I have to decline the patch again but running those tests with the > latest 3.6.8 release or a Namoroka build always gives me 15 failures. See the > following report: > > http://brasstacks.mozilla.com/couchdb/mozmill-crowd/_design/dashboard/_show/report/d37f8f06049accd405a3f2b55df5294d > > All details see: > http://brasstacks.mozilla.com/couchdb/mozmill-crowd/d37f8f06049accd405a3f2b55df5294d > Those reports are very clear in their layout. Cool to have such a good feedback! From what I see, there are roughly 2 kinds of errors: 1. "Components.interfaces.nsIAccessibleProvider is undefined" which seems to be a problem on Mac, see "jumlib assertNotNull not working correctly on Mac" http://groups.google.com/group/mozmill-dev/browse_thread/thread/10862025cea22ffd Henrik, could you do something about it somehow? 2. Some other Mac-specific or Linux-specific testing, since I'm only testing on Linux yet. I'll be trying to test on Windows to have the test results from another platform than Linux. But this will take me some time to setup a good development environment on Windows (not mentioning the pleasure of it). Unfortunately I haven't any virtual system running Mac OS. > Also please remove the code which creates the fidesfit-client.debug file in the > users home directory. We do not want to touch any other directories of the > users system. I understand that this is problematic. I'll provide this debug information to the user in another way, for example through a web page. Thanks for pointing it out.

(In reply to comment #30) > From what I see, there are roughly 2 kinds of errors: > > 1. "Components.interfaces.nsIAccessibleProvider is undefined" which seems to be > a problem on Mac, see "jumlib assertNotNull not working correctly on Mac" > http://groups.google.com/group/mozmill-dev/browse_thread/thread/10862025cea22ffd nsIAccessibleProvider is not available on Mac. But I wonder from where this problem comes from. I have answered Ankush's question but I would need a simpler testcase. I wonder if something inside the frame is trying to instantiate nsIAccessibleProvider. > I'll be trying to test on Windows to have the test results from another > platform than Linux. But this will take me some time to setup a good > development environment on Windows (not mentioning the pleasure of it). > Unfortunately I haven't any virtual system running Mac OS. I could try to start a testrun later today on Windows. So I would like that you concentrate on the above failure and eventually work together with Ankush. We should try to find out this problem. > I understand that this is problematic. I'll provide this debug information to > the user in another way, for example through a web page. Thanks for pointing it > out. You can also use dump() to directly log to the console which will be part of the log file you can specify with "--log=path.

Results for windows: http://brasstacks.mozilla.com/couchdb/mozmill-crowd/_design/dashboard/_show/report/d37f8f06049accd405a3f2b55df53e89 All details: http://brasstacks.mozilla.com/couchdb/mozmill-crowd/d37f8f06049accd405a3f2b55df53e89 As what I can see the File menu gets opened twice and not closed anymore. Evntually that could be the reason of some failures.

The problem with the nsIAccessible failure is located here: get_accessibleType()@chrome://global/content/bindings/menu.xml:18 ClassLister([object XULElement])@file:///private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpWaW0VI.mozrunner/extensions/fidesfit-client@fidesfit.org/modules/ClassLister.jsm:70 ("file-menu") Looks like it's the problem I was seeing before. Because of it the file menu keeps staying open. What are you doing in that line?

(In reply to comment #33) > The problem with the nsIAccessible failure is located here: > > get_accessibleType()@chrome://global/content/bindings/menu.xml:18 > ClassLister([object > XULElement])@file:///private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpWaW0VI.mozrunner/extensions/fidesfit-client@fidesfit.org/modules/ClassLister.jsm:70 > So this is the actual copy of all properties of a DOM element to an instance of a function that fails: // Binding all the properties of the wrapped object to the container for (var property_name in element) { if (property_name == CLASSLIST_PROP) continue; this[property_name] = element[property_name]; } > Looks like it's the problem I was seeing before. Because of it the file menu > keeps staying open. > Acknowledged > What are you doing in that line? I am trying to copy all properties of the wrapped DOM element to its wrapper, so that the wrapper (the ClassLister instance) can do exactly the same actions that a DOM element does, *plus* the CSS-classes-specific actions that are available in DOM element but only on Firefox >= 3.6 It seems there are cleaner ways to do object inheritance (which is basically what I'm trying to do) in new JavaScript versions but those will only be available for Firefox >= 4. And my point is precisely to use CSS-classes-specific actions for Firefox versions from 3.5 included.

(In reply to comment #33) > The problem with the nsIAccessible failure is located here: > > get_accessibleType()@chrome://global/content/bindings/menu.xml:18 > ClassLister([object > XULElement])@file:///private/var/folders/85/85YEzSCiEiyDDPnVLLhW3k+++TI/-Tmp-/tmpWaW0VI.mozrunner/extensions/fidesfit-client@fidesfit.org/modules/ClassLister.jsm:70 > ("file-menu") > > Looks like it's the problem I was seeing before. Because of it the file menu > keeps staying open. What are you doing in that line? The file menu stays open a lot on my Linux platform too which doesn't have the problem with the Components.interfaces.nsIAccessibleProvider. Since the tests were passing I didn't bother much with this menu being open some of the time. And it seems to me that I've had this file menu often open since the beginning, even before I had developed the ClassLister function.

Which action do you perform so the menu stays open? I'm kinda worried about possible failures because of focus or overlaying issues.

(In reply to comment #36) > Which action do you perform so the menu stays open? In many tests I do open a tab by doing so: controller.click(menu_item); var menu_item = new elementslib.ID(controller.window.document, 'menu_newNavigatorTab'); controller.click(menu_item); controller.open(url); And all the time the file menu stays open when opened through Mozmill. While when the same action is done manually, the file menu closes itself when one clicks the "new tab" menu item.

(In reply to comment #37) > (In reply to comment #36) > > Which action do you perform so the menu stays open? > > In many tests I do open a tab by doing so: > Sorry, here is the full code: var menu_item; menu_item = new elementslib.ID(controller.window.document, 'file-menu'); controller.click(menu_item); menu_item = new elementslib.ID(controller.window.document, 'menu_newNavigatorTab'); controller.click(menu_item); controller.open(url);

This new version of the patch provides the following improvements: - Fixed SQLite default file copy logic big bug which was causing many of the tests on Windows (if not all) - Tests updated to use new Mozmill API and shared modules - Less sleep calls and extensive use of the "waitFor" method to have as intelligent and resilient tests as possible - No more debug file write in the user directory. It is now an HTML page displayed in the browser. I have now a very basic environment to test on Windows, which helped me to find the bug regarding file copying.

This new version of the patch points to fidesfit-client-0.21.0.xpi which adds official support for Firefox 4.0. The problem with Firefox 4.0 was the introduction of the new AddonManager and the total removal (instead of deprecation) of the ExtensionManager.

Comment on attachment 474500 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension >+linux=https://download.fidesfit.org/browser-extensions/firefox/fidesfit-client/fidesfit-client-0.21.0.xpi >+mac=https://download.fidesfit.org/browser-extensions/firefox/fidesfit-client/fidesfit-client-0.21.0.xpi >+win=https://download.fidesfit.org/browser-extensions/firefox/fidesfit-client/fidesfit-client-0.21.0.xpi Sadly those XPI's aren't compatible with b6 or b7pre. >+ // Waiting to effectively be on the next tab >+ controller.waitFor(function() { >+ return controller.tabs.activeTabIndex == active_tab_index++; >+ }); You will also have to define a timeout for waitFor as you already do for waitForEval. Right now some tests wait 30s. I still have a couple of failures: http://brasstacks.mozilla.com/couchdb/mozmill-crowd/_design/dashboard/_show/report/0e34faa0c328ca9572aba8cbcca1d07a 1. testConfigureWindow & testUserIdNormalization The configuration dialog doesn't close anymore. Also no text in inputted so I assume we fail right before. 2. To use the main menu please use controller.click(new elementslib.Elem(controller.menus["file-menu"].menu_newNavigatorTab). 3. Regarding the nsIAccessible problem, can you disable all tests which are using the ClassLister on OS X?

(In reply to comment #41) > > 1. testConfigureWindow & testUserIdNormalization > > The configuration dialog doesn't close anymore. Also no text in inputted so I > assume we fail right before. > > 2. To use the main menu please use controller.click(new > elementslib.Elem(controller.menus["file-menu"].menu_newNavigatorTab). > > 3. Regarding the nsIAccessible problem, can you disable all tests which are > using the ClassLister on OS X? I'm on the way of getting a second hand Mac OS X system. That should speed up things.

Thanks for the heads-up. It sounds great! Also keep in mind that we are working hard at the moment to refactor all of our shared modules. At some point you will have to update your tests accordingly. That's the reason why I haven't contacted you in the last weeks. Current ETA is set for end of this quarter.

Here is at last a new version of the Mozmill tests for the fidesfit-client addon. Those tests pass on Firefox 4.0 and Firefox 3.6 on both Linux and Mac OS X (on the second hand Mac OS X system devoted for Mozmill tests mentioned above). I must admit that I haven't have the will and time to test again on Windows. There are 3 other remaining Mozmill test files that I have not included in the patch since their tests don't pass on Mac yet and I might request help for that. But I would rather finally have fewer tests checked-in into the Mozilla infrastructure than none at all. So if you find any more problem, let's just remove the problematic files and go ahead. I'm always very much interested in having those addon tests automatically checked against the Firefox builds and have all your Mozmill-experts advises and thus benefiting from this QA.

I should added that the tests in the patch have been tested with Mozmill 1.5.3 and the patch is against the aurora branch of the mozmill-tests repository.

To follow the latest tree structure change and new name conventions, this new patch puts the given tests in the right directory and now on uses the "lib" directory instead of the "shared-modules".

Comment on attachment 526669 [details] [diff] [review] Patch against mozmill-tests to add addon tests for fidesfit-client extension Thanks for the updated patch and your proposal to let those tests get checked-in, even with a subset of tests. That's something I like and also propose. Before I check this patch in detail, some advices because a lot has been changed since you have uploaded the last revision of the tests. >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/dom-utils.js Mon Apr 18 09:53:03 2011 +0200 >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/modal-dialog.js Mon Apr 18 09:53:03 2011 +0200 >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/prefs.js Mon Apr 18 09:53:03 2011 +0200 >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/tabs.js Mon Apr 18 09:53:03 2011 +0200 >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/utils.js Mon Apr 18 09:53:03 2011 +0200 There is no need for you anymore to fork those shared modules. You can now require them from whereever you want across folder boundaries. >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/fidesfit_helper.js Mon Apr 18 09:53:03 2011 +0200 >+ * Portions created by the Initial Developer are Copyright (C) 2010-2011 nit: We normally only use the year when it has been checked in. So 2011 will be enough. >+ handleWindow: function fidesfitHelper_handleWindow(type, text, callback, dontClose) { >+ return utils.handleWindow(type, text, callback, dontClose); >+ }, Doesn't seem to be needed anymore. >+ openInNewTab: function fidesfitHelper_openInNewTab(url) { >+ //var event = { type: 'newTabButton' }; >+ var event = { type: 'menu' }; >+ this._tabBrowser.openTab(event); >+ >+ // So using our own way of opening tabs >+ // var file_menu_item = new elementslib.ID(this._controller.window.document, >+ // 'file-menu'); >+ // var new_tab_menu_item = new elementslib.ID(this._controller.window.document, >+ // 'menu_newNavigatorTab'); >+ // this._controller.click(file_menu_item); >+ // this._controller.click(new_tab_menu_item); >+ >+ this._controller.open(url); Please use the appropriate function from the TabBrowser class directly. I don't see a need for another wrapper. >+ assertArrayEquals: function fidesfitHelper_assertArrayEquals(value1, value2, >+ comment) { >+ if (value1.length != value2.length) { >+ return false; >+ } >+ >+ for (var i = 0; i < value1.length; i++) { >+ if ((value1[i] != value2[i]) || (typeof value1[i] != typeof value2[i])) { >+ return false; >+ } >+ } >+ return true; >+ }, It's not an assert method because it doesn't assert anything but return a boolean value. It's a bit misleading. >+ var js_loader = Cc['@mozilla.org/moz/jssubscript-loader;1']. >+ getService(Ci.mozIJSSubScriptLoader); >+ // Even if not called directly in this method, we need to load ui.js >+ // because it is later on use by client.js. >+ js_loader.loadSubScript('chrome://fidesfit-client/content/ui.js', >+ { Cc: Cc, Ci: Ci, Cu: Cu, fidesfit: fidesfit }); >+ js_loader.loadSubScript('chrome://fidesfit-client/content/client.js', >+ { Cc: Cc, Ci: Ci, Cu: Cu, fidesfit: fidesfit }); This snippet is used a couple of times and should probably moved into a module for proper handling. >+ // Waiting until there are some user messages retrieved >+ // TODO: This sleep shouldn't be needed but it is :'( >+ controller.sleep(5000); Can we get rid of this? Are there any events synthesized when an user message arrives? >+ controller.waitFor(function() { >+ return state.getAllUserMessagesCount() != 0; >+ }, >+ null, 2000 The parameters the waitFor function expects have been changed. The 2nd param is now a message. So the Selenium tests as an example. Also you should try to move to triple operators in the future for comparisons. >+ // Verifying that a visual notification appears >+ controller.sleep(1000); >+ var elem = fidesfit_helper.getElement({ type: 'statusbar-opinion' }).getNode(); You should really wait for the notification to be open. Which type of notification is it? Something like the password save reminder? If yes, check our password not saved test.

This patch addresses the points raised in comment #47.

(In reply to comment #47) > > >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/dom-utils.js Mon Apr 18 09:53:03 2011 +0200 > >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/modal-dialog.js Mon Apr 18 09:53:03 2011 +0200 > >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/prefs.js Mon Apr 18 09:53:03 2011 +0200 > >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/tabs.js Mon Apr 18 09:53:03 2011 +0200 > >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/utils.js Mon Apr 18 09:53:03 2011 +0200 > > There is no need for you anymore to fork those shared modules. You can now > require them from whereever you want across folder boundaries. > Right. Done. I didn't do it before just because I hadn't the time and I wanted to show you good signs of progress. > >+++ b/tests/addons/fidesfit-client@fidesfit.org/lib/fidesfit_helper.js Mon Apr 18 09:53:03 2011 +0200 > >+ * Portions created by the Initial Developer are Copyright (C) 2010-2011 > > nit: We normally only use the year when it has been checked in. So 2011 will be > enough. > Done. But with year 2010 since this is the first year of publication of this code on SourceForge. > >+ handleWindow: function fidesfitHelper_handleWindow(type, text, callback, dontClose) { > >+ return utils.handleWindow(type, text, callback, dontClose); > >+ }, > > Doesn't seem to be needed anymore. > It is needed for another important test file that doesn't pass on Mac OS X yet. This important test file involves preferences, prefwindow and window closings. I'm working on it and will propose it as soon as it works. But it's better to have an initial landing now than a potential landing someday. > >+ openInNewTab: function fidesfitHelper_openInNewTab(url) { > >+ //var event = { type: 'newTabButton' }; > >+ var event = { type: 'menu' }; > >+ this._tabBrowser.openTab(event); > >+ > >+ // So using our own way of opening tabs > >+ // var file_menu_item = new elementslib.ID(this._controller.window.document, > >+ // 'file-menu'); > >+ // var new_tab_menu_item = new elementslib.ID(this._controller.window.document, > >+ // 'menu_newNavigatorTab'); > >+ // this._controller.click(file_menu_item); > >+ // this._controller.click(new_tab_menu_item); > >+ > >+ this._controller.open(url); > > Please use the appropriate function from the TabBrowser class directly. I don't > see a need for another wrapper. > In tabs.js the openInNewTab method doesn't take an URL as an argument. Thus having a wrapper is still legitimate isn't it? > >+ assertArrayEquals: function fidesfitHelper_assertArrayEquals(value1, value2, > >+ comment) { > >+ if (value1.length != value2.length) { > >+ return false; > >+ } > >+ > >+ for (var i = 0; i < value1.length; i++) { > >+ if ((value1[i] != value2[i]) || (typeof value1[i] != typeof value2[i])) { > >+ return false; > >+ } > >+ } > >+ return true; > >+ }, > > It's not an assert method because it doesn't assert anything but return a > boolean value. It's a bit misleading. > In bug 568978 there is a patch to add this method directly into Mozmill. So as soon as this patch lands, I'll remove this method. So let's forget it for now. > >+ var js_loader = Cc['@mozilla.org/moz/jssubscript-loader;1']. > >+ getService(Ci.mozIJSSubScriptLoader); > >+ // Even if not called directly in this method, we need to load ui.js > >+ // because it is later on use by client.js. > >+ js_loader.loadSubScript('chrome://fidesfit-client/content/ui.js', > >+ { Cc: Cc, Ci: Ci, Cu: Cu, fidesfit: fidesfit }); > >+ js_loader.loadSubScript('chrome://fidesfit-client/content/client.js', > >+ { Cc: Cc, Ci: Ci, Cu: Cu, fidesfit: fidesfit }); > > This snippet is used a couple of times and should probably moved into a module > for proper handling. > Good idea. Done. > >+ // Waiting until there are some user messages retrieved > >+ // TODO: This sleep shouldn't be needed but it is :'( > >+ controller.sleep(5000); > > Can we get rid of this? Are there any events synthesized when an user message > arrives? > Done. > >+ controller.waitFor(function() { > >+ return state.getAllUserMessagesCount() != 0; > >+ }, > >+ null, 2000 > > The parameters the waitFor function expects have been changed. The 2nd param is > now a message. Yes, I've seen. Notice there that the message is null. Thus the way I've used waitFor seems right to me. > Also you should try to move to triple operators in the future for comparisons. > For numbers too??? > >+ // Verifying that a visual notification appears > >+ controller.sleep(1000); > >+ var elem = fidesfit_helper.getElement({ type: 'statusbar-opinion' }).getNode(); > > You should really wait for the notification to be open. Which type of > notification is it? Something like the password save reminder? If yes, check > our password not saved test. Right. Done.

(In reply to comment #49) > > >+ handleWindow: function fidesfitHelper_handleWindow(type, text, callback, dontClose) { > > >+ return utils.handleWindow(type, text, callback, dontClose); > > >+ }, > > > > Doesn't seem to be needed anymore. > > > > It is needed for another important test file that doesn't pass on Mac OS X > yet. > This important test file involves preferences, prefwindow and window > closings. > I'm working on it and will propose it as soon as it works. But it's better > to have an initial landing now than a potential landing someday. It's just a 1:1 wrapper for our handleWindow method in utils.js. Why do you need this extra wrapper? > In tabs.js the openInNewTab method doesn't take an URL as an argument. Thus > having a wrapper is still legitimate isn't it? So it should just be a one-liner in your tests for open a new tab and calling controller.open()? You should probably rename it then to reduce confusion with our method in tabs.js. > > Also you should try to move to triple operators in the future for comparisons. > > > > For numbers too??? Sure. "'2' == 2" and "'2' === 2" are quite different because both are of different type. I will wait for the above response before reviewing the patch. The above mentioned topics are enhancements. You can follow-up in new bugs, but then please create those.

As requested: - From now on, using only strict comparison operators - Removed handleWindow method from fidesfit_helper which was just a 1:1 wrapper - - Renamed fidesfit_helper.openInNewTab into fidesfit_helper.openUrlInNewTab to avoid confusion with mozmill-tests shared modules

Marc, can you please tell me on which platforms you have tested with which versions of Firefox? I just want to do a test on missing platforms for the hopefully last round of review on this bug. Thanks.

(In reply to comment #52) > Marc, can you please tell me on which platforms you have tested with which > versions of Firefox? I just want to do a test on missing platforms for the > hopefully last round of review on this bug. Thanks. I had tested with only Firefox 4.0 on Linux and Mac OS X. I didn't test on Windows, which I have just done now and found that a test is failing :-( So I'll post yet another patch to have this fixed. But this patch will be a test on a new version of the extension. I haven't time enough to have a branch to follow an old version of the extension as of yet, at least not before there is an actual initial code landing.

Thanks Marc, what is the ETA for the new version? Means even with the upcoming patch reviewed we will have to wait for the new version to be released? Otherwise we could strip the test which is failing on Windows and deliver it with the update for the next version?

This patch provides tests that pass on Linux, Mac OS X, Windows with Firefox 4.0 + Mozmill 1.5.3 Notes: - It was really needed to finalized this new version since the test that was failing on Windows was really a problem that needed to be fixed. - The test_statusbar.testIndicatorAction test has failed some times on Mac without any reason that I could find while it was passing fine on Linux and Windows. To avoid this problem I've added a "if (mozmill.isMac) return;" block at the beginning of this test, but it is currently commented out since the test passes on my Mac system

Comment on attachment 533586 [details] [diff] [review] Patch (2011-05-19-01) against mozmill-tests to add addon tests for fidesfit-client extension Thanks for the update. I think we are now in a very good shape to get it landed. I did another test-run with Firefox 4.0 and can second that everything passes. I will now check it into the mozilla-2.0 branch. But you should check now how it works for mozilla-beta, mozilla-aurora, or even default. Would be good to get it landed there too.

Wait, is it expected that you haven't given any email address? We should really have one so we know whom to contact. When you update the patch please also add a commit message and export the patch via 'hg export tip >file'.

Not sure why this bug was under extension compatibility. Moving to Mozmill Tests.

First, I'm very glad the review is granted! :-) Thank you. And thank you for your help all the way that led to this. (In reply to comment #57) > Wait, is it expected that you haven't given any email address? We should > really have one so we know whom to contact. > Yes I will, a developer of Mozilla Europe told me so last week. > When you update the patch please > also add a commit message and export the patch via 'hg export tip >file'. > At the moment I basically do the following (through a Makefile): cp -R test/mozmill/tests ${MOZMILL_TESTS_LOCAL_REPO_EXTENSION_DIR_PATH} cp -R test/mozmill/lib ${MOZMILL_TESTS_LOCAL_REPO_EXTENSION_DIR_PATH} hg add ${MOZMILL_TESTS_LOCAL_REPO_EXTENSION_DIR_PATH}; hg diff > ${MOZMILL_TESTS_PATCH_PATH} I don't see how I could replace this by 'hg export tip >file'. Could you elaborate a bit more please?

(In reply to comment #56) > Comment on attachment 533586 [details] [diff] [review] [review] > Patch (2011-05-19-01) against mozmill-tests to add addon tests for > fidesfit-client extension > > Thanks for the update. I think we are now in a very good shape to get it > landed. I did another test-run with Firefox 4.0 and can second that > everything passes. > > I will now check it into the mozilla-2.0 branch. But you should check now > how it works for mozilla-beta, mozilla-aurora, or even default. Would be > good to get it landed there too. Truly naive and non-malevolent question of mine: Hasn't the QA a big automated test infrastructure that just swallow a patch and tests it against every kind of possible setup? And when I test fidesfit-client against mozilla-beta, mozilla-aurora, and default, should I just tell you whether it works? I would like a bit more understanding on what will be the lifecycle of developments regarding this extension, what is the work you are expecting me to do regularly?

(In reply to comment #59) > hg diff > ${MOZMILL_TESTS_PATCH_PATH} > > I don't see how I could replace this by 'hg export tip >file'. Could you > elaborate a bit more please? Replace the above command so it is: hg export tip > ${MOZMILL_TESTS_PATCH_PATH} That way you will get some lines of comments at the top of the file with your user name and the commit message. See the following link for the format of the commit message: https://developer.mozilla.org/en/Mozmill_Tests#Commit_Message (In reply to comment #60) > Truly naive and non-malevolent question of mine: Hasn't the QA a big > automated test infrastructure that just swallow a patch and tests it against > every kind of possible setup? We don't have a tryserver like pre-testing environment yet. So it would also be manual testing. > And when I test fidesfit-client against mozilla-beta, mozilla-aurora, and > default, should I just tell you whether it works? I would like a bit more > understanding on what will be the lifecycle of developments regarding this > extension, what is the work you are expecting me to do regularly? Yes, the extension has to be compatible with the version of Firefox on those branches and no failures have to happen. Only in such a state we can check-in the patch on those branches. It's up to your policy which versions of Firefox the extension supports. But keep in mind that as early as possible we could detect regressions on both sides, we can also fix those regressions. Speaking of failures, you will be in charge for the quality of tests. If they start failing you should fix those. Probably by next month I will add a daily test-run for add-ons across branches and platforms, which will result in daily reports. Also once checked-in users of the Mozmill Crowd extension will be able to run those tests and report the results.

New patch features: - Email address addition in all file headers - Moving of the net mockup code out of the fidesfit_helper into its own lib/net.js module for more legible code - Usage of Mercurial Queue for the patch The tests pass on Linux, Mac, Windows for Firefox 4 with Mozmill 1.5.4b3 + patch. The tests pass on Linux and Mac for Firefox Beta with Mozmill 1.5.4b3 + patch. The tests pass on Linux for Firefox Aurora with Mozmill 1.5.4b3 + patch. Testing on Windows with command-line Mozmill produces errors and will ask for help about it on the mailing list.

New patch features: - Fixes in previous tests that were wrongly asserting (assertArrayEquals and some other) - More new tests that passes on both Linux, Mac OS X, Windows - Simplifications and removal of unused code - Made to work with extension version 0.39.0 which features more modularization into JavaScript modules and more documented code

Please don't add any more tests or major changes. We should really get this in. New and updated code across the whole patch makes it even harder. So please keep the number of tests the same until this patch has been checked in. Thanks.

(In reply to comment #64) > Please don't add any more tests or major changes. We should really get this > in. New and updated code across the whole patch makes it even harder. So > please keep the number of tests the same until this patch has been checked > in. Thanks. OK. Sorry. I won't touch this ticket unless you request it. Still, the new version of the patch is easier to read and review than the last one ;-) But OK OK, I won't touch it again unless requested.

Comment on attachment 544047 [details] [diff] [review] Patch (2011-07-05) against mozmill-tests to add addon tests for fidesfit-client extension That's a lot of code changes. :/ So before I want to check any of those I did a test-run on OS X and it failed in two cases: http://mozmill-archive.brasstacks.mozilla.com/#/addons/report/ca2afbf1bd5ce4e8e57d4acd64b7ad51 Please get those fixed and re-run on your side. It would be helpful if you could send reports to the above database and include the URLs in the next review request.

(In reply to comment #66) > Comment on attachment 544047 [details] [diff] [review] [review] > Patch (2011-07-05) against mozmill-tests to add addon tests for > fidesfit-client extension > > That's a lot of code changes. :/ > Yes I know. But that make the code really more modular, more documented and also fixed some bugs in the tests. > So before I want to check any of those I > did a test-run on OS X and it failed in two cases: > > http://mozmill-archive.brasstacks.mozilla.com/#/addons/report/ > ca2afbf1bd5ce4e8e57d4acd64b7ad51 > > Please get those fixed and re-run on your side. It would be helpful if you > could send reports to the above database and include the URLs in the next > review request. > The fidesfit-client@fidesfit.org/tests/test_statusbar.js test file can be removed, it is a test that sometimes pass, sometimes fail. Its behavior is not predictable enough, especially on Mac. I had told you in comment #55. But let's remove it altogether until later. As for the /fidesfit-client@fidesfit.org/tests/test_module_orderedmap.js testNewFromJson test it's really interesting: it was passing by the time I did submit the patch, but something has changed on Aurora native JSON and the method I am using have disappeared. I'm investigating it. Could you still review+ considering that this test still passes on Firefox stable and Firefox beta?

Ah, right. There was a change on mozilla-central before the merge re: native JSON. It has then be merged into Aurora. I don't have the bug number handy. Could you please update the patch and just remove the statusbar test? I will then do another testrun on beta and release, and review the patch. Thanks.

(In reply to comment #68) > Ah, right. There was a change on mozilla-central before the merge re: native > JSON. It has then be merged into Aurora. I don't have the bug number handy. > > Could you please update the patch and just remove the statusbar test? I will > then do another testrun on beta and release, and review the patch. Thanks. I've found how to adapt to the native JSON change (which by the way improves greatly how to deal with native JSON from JavaScript modules). I've fixed the current version of the fidesfit-client extension to this end. I'll modify the patch as you are requesting and will post it for review again asap.

This patch: - fixes the native JSON problem - removes 2 tests to be sure that the rest will pass without problem - relies on a new online fixed version of fidesfit-client-0.39.0.xpi I am now using branches in the fidesfit-client code repository to be able to do separate fixes wrt Mozmill.

Comment on attachment 547226 [details] [diff] [review] Patch (2011-07-20) against mozmill-tests to add addon tests for fidesfit-client extension Ok, that looks good and I'm eager to try to land it today on all the new rapid release branches (default, aurora, beta, and release). Starting with today we will have daily test-runs for add-ons so it's a perfect time for your tests being landed. Also there are some things which needs improvements, but aren't blockers. So as discussed you can take care of in the next bugs which will hopefully be a bit smaller. Thanks for your hard work Marc!

Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/d166ccf07d77 (default) http://hg.mozilla.org/qa/mozmill-tests/rev/4694c289c7f0 (aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/5f742d48e570 (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/54ab2b912c07 (release) Lets wait for the today's test results.