Last Comment Bug 570493 - fidesfit-client tests initial landing
: fidesfit-client tests initial landing
Status: RESOLVED FIXED
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: ---
Assigned To: Marc-Aurèle DARCHE
:
Mentors:
http://sourceforge.net/projects/fides...
Depends on:
Blocks: 568958
  Show dependency treegraph
 
Reported: 2010-06-07 08:55 PDT by Marc-Aurèle DARCHE
Modified: 2011-07-22 06:10 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch against mozmill-tests to add addon tests for fidesfit-client extension (43.43 KB, patch)
2010-06-07 09:33 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (60.66 KB, patch)
2010-06-09 03:38 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (62.57 KB, patch)
2010-06-14 09:14 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (61.24 KB, patch)
2010-06-15 08:23 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (67.68 KB, patch)
2010-07-01 12:24 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (77.25 KB, patch)
2010-07-02 13:01 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (87.13 KB, patch)
2010-07-17 01:51 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (112.70 KB, patch)
2010-08-26 03:33 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (118.46 KB, patch)
2010-09-10 04:18 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (118.43 KB, patch)
2010-09-12 03:36 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (131.73 KB, patch)
2011-04-17 13:29 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (131.70 KB, patch)
2011-04-18 00:55 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (55.58 KB, patch)
2011-04-28 12:46 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch against mozmill-tests to add addon tests for fidesfit-client extension (54.55 KB, patch)
2011-05-13 05:52 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch (2011-05-19-01) against mozmill-tests to add addon tests for fidesfit-client extension (57.52 KB, patch)
2011-05-19 04:04 PDT, Marc-Aurèle DARCHE
hskupin: review+
Details | Diff | Review
Patch (2011-06-20-01) against mozmill-tests to add addon tests for fidesfit-client extension (68.13 KB, patch)
2011-06-20 10:42 PDT, Marc-Aurèle DARCHE
no flags Details | Diff | Review
Patch (2011-07-05) against mozmill-tests to add addon tests for fidesfit-client extension (79.62 KB, patch)
2011-07-05 13:32 PDT, Marc-Aurèle DARCHE
hskupin: review-
Details | Diff | Review
Patch (2011-07-20) against mozmill-tests to add addon tests for fidesfit-client extension (66.93 KB, patch)
2011-07-20 13:38 PDT, Marc-Aurèle DARCHE
hskupin: review+
Details | Diff | Review

Description Marc-Aurèle DARCHE 2010-06-07 08:55:25 PDT
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
Comment 1 Marc-Aurèle DARCHE 2010-06-07 09:33:52 PDT
Created attachment 449646 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension
Comment 2 Henrik Skupin (:whimboo) 2010-06-07 10:03:13 PDT
If the patch is ready for review please ask for it on the attachment.
Comment 3 Marc-Aurèle DARCHE 2010-06-07 11:34:17 PDT
Comment on attachment 449646 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

Trying to ask for review
Comment 4 Marc-Aurèle DARCHE 2010-06-07 11:36:45 PDT
Henrik, have I managed to properly ask the patch for review?
Comment 5 Marc-Aurèle DARCHE 2010-06-09 03:32:08 PDT
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 6 Marc-Aurèle DARCHE 2010-06-09 03:38:35 PDT
Created attachment 450081 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension
Comment 7 Henrik Skupin (:whimboo) 2010-06-13 13:50:10 PDT
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.
Comment 8 Marc-Aurèle DARCHE 2010-06-14 06:30:08 PDT
(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 9 Henrik Skupin (:whimboo) 2010-06-14 06:33:48 PDT
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.
Comment 10 Marc-Aurèle DARCHE 2010-06-14 09:14:50 PDT
Created attachment 451064 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

I have acted upon all your requests, except the point related to the shared modules to which I will reply in another comment.
Comment 11 Marc-Aurèle DARCHE 2010-06-14 09:16:54 PDT
(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 12 Henrik Skupin (:whimboo) 2010-06-14 10:21:38 PDT
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
Comment 13 Marc-Aurèle DARCHE 2010-06-15 08:23:09 PDT
Created attachment 451288 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

This new patch has the shared module inclusion in the FidesfitHelper as you requested.
Comment 14 Henrik Skupin (:whimboo) 2010-06-28 16:31:38 PDT
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.
Comment 15 Marc-Aurèle DARCHE 2010-06-30 21:57:06 PDT
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 :-)
Comment 16 Henrik Skupin (:whimboo) 2010-07-01 00:41:15 PDT
(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.
Comment 17 Marc-Aurèle DARCHE 2010-07-01 00:49:49 PDT
(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.
Comment 18 Marc-Aurèle DARCHE 2010-07-01 06:29:31 PDT
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.
Comment 19 Marc-Aurèle DARCHE 2010-07-01 08:48:00 PDT
(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 :-\
Comment 20 Henrik Skupin (:whimboo) 2010-07-01 09:36:11 PDT
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.
Comment 21 Marc-Aurèle DARCHE 2010-07-01 12:24:55 PDT
Created attachment 455516 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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 :-)
Comment 22 Marc-Aurèle DARCHE 2010-07-02 13:01:01 PDT
Created attachment 455756 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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.
Comment 23 Marc-Aurèle DARCHE 2010-07-17 01:51:30 PDT
Created attachment 458069 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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 24 Henrik Skupin (:whimboo) 2010-07-29 06:31:47 PDT
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.
Comment 25 Marc-Aurèle DARCHE 2010-08-26 03:33:08 PDT
Created attachment 469412 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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.
Comment 26 Henrik Skupin (:whimboo) 2010-09-02 03:44:35 PDT
(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.
Comment 27 Marc-Aurèle DARCHE 2010-09-02 03:55:47 PDT
(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 28 Henrik Skupin (:whimboo) 2010-09-02 06:29:30 PDT
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.
Comment 29 Henrik Skupin (:whimboo) 2010-09-02 06:32:10 PDT
(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.
Comment 30 Marc-Aurèle DARCHE 2010-09-02 06:48:33 PDT
(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.
Comment 31 Henrik Skupin (:whimboo) 2010-09-02 08:00:49 PDT
(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.
Comment 32 Henrik Skupin (:whimboo) 2010-09-02 08:13:21 PDT
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.
Comment 33 Henrik Skupin (:whimboo) 2010-09-05 16:45:34 PDT
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?
Comment 34 Marc-Aurèle DARCHE 2010-09-07 06:14:11 PDT
(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.
Comment 35 Marc-Aurèle DARCHE 2010-09-07 08:48:57 PDT
(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.
Comment 36 Henrik Skupin (:whimboo) 2010-09-08 14:51:06 PDT
Which action do you perform so the menu stays open? I'm kinda worried about possible failures because of focus or overlaying issues.
Comment 37 Marc-Aurèle DARCHE 2010-09-09 06:30:04 PDT
(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.
Comment 38 Marc-Aurèle DARCHE 2010-09-09 06:31:55 PDT
(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);
Comment 39 Marc-Aurèle DARCHE 2010-09-10 04:18:07 PDT
Created attachment 474009 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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.
Comment 40 Marc-Aurèle DARCHE 2010-09-12 03:36:22 PDT
Created attachment 474500 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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 41 Henrik Skupin (:whimboo) 2010-09-22 07:39:16 PDT
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?
Comment 42 Marc-Aurèle DARCHE 2011-02-08 04:45:18 PST
(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.
Comment 43 Henrik Skupin (:whimboo) 2011-02-08 05:15:30 PST
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.
Comment 44 Marc-Aurèle DARCHE 2011-04-17 13:29:05 PDT
Created attachment 526623 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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.
Comment 45 Marc-Aurèle DARCHE 2011-04-18 00:40:58 PDT
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.
Comment 46 Marc-Aurèle DARCHE 2011-04-18 00:55:37 PDT
Created attachment 526669 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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 47 Henrik Skupin (:whimboo) 2011-04-26 09:32:30 PDT
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.
Comment 48 Marc-Aurèle DARCHE 2011-04-28 12:46:47 PDT
Created attachment 528935 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

This patch addresses the points raised in comment #47.
Comment 49 Marc-Aurèle DARCHE 2011-04-28 13:04:31 PDT
(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.
Comment 50 Henrik Skupin (:whimboo) 2011-05-13 03:41:12 PDT
(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.
Comment 51 Marc-Aurèle DARCHE 2011-05-13 05:52:26 PDT
Created attachment 532181 [details] [diff] [review]
Patch against mozmill-tests to add addon tests for fidesfit-client extension

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
Comment 52 Henrik Skupin (:whimboo) 2011-05-17 05:26:50 PDT
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.
Comment 53 Marc-Aurèle DARCHE 2011-05-18 08:08:27 PDT
(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.
Comment 54 Henrik Skupin (:whimboo) 2011-05-18 11:21:29 PDT
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?
Comment 55 Marc-Aurèle DARCHE 2011-05-19 04:04:40 PDT
Created attachment 533586 [details] [diff] [review]
Patch (2011-05-19-01) against mozmill-tests to add addon tests for fidesfit-client extension

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 56 Henrik Skupin (:whimboo) 2011-05-26 13:27:02 PDT
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.
Comment 57 Henrik Skupin (:whimboo) 2011-05-26 13:29:00 PDT
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'.
Comment 58 Henrik Skupin (:whimboo) 2011-05-26 13:30:19 PDT
Not sure why this bug was under extension compatibility. Moving to Mozmill Tests.
Comment 59 Marc-Aurèle DARCHE 2011-05-26 13:50:03 PDT
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?
Comment 60 Marc-Aurèle DARCHE 2011-05-26 13:55:34 PDT
(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?
Comment 61 Henrik Skupin (:whimboo) 2011-05-26 15:00:34 PDT
(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.
Comment 62 Marc-Aurèle DARCHE 2011-06-20 10:42:14 PDT
Created attachment 540526 [details] [diff] [review]
Patch (2011-06-20-01) against mozmill-tests to add addon tests for fidesfit-client extension

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.
Comment 63 Marc-Aurèle DARCHE 2011-07-05 13:32:04 PDT
Created attachment 544047 [details] [diff] [review]
Patch (2011-07-05) against mozmill-tests to add addon tests for fidesfit-client extension

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
Comment 64 Henrik Skupin (:whimboo) 2011-07-06 14:41:03 PDT
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.
Comment 65 Marc-Aurèle DARCHE 2011-07-07 00:50:15 PDT
(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 66 Henrik Skupin (:whimboo) 2011-07-19 03:53:33 PDT
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.
Comment 67 Marc-Aurèle DARCHE 2011-07-20 03:03:24 PDT
(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?
Comment 68 Henrik Skupin (:whimboo) 2011-07-20 04:53:17 PDT
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.
Comment 69 Marc-Aurèle DARCHE 2011-07-20 05:08:49 PDT
(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.
Comment 70 Marc-Aurèle DARCHE 2011-07-20 13:38:22 PDT
Created attachment 547226 [details] [diff] [review]
Patch (2011-07-20) against mozmill-tests to add addon tests for fidesfit-client extension

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 71 Henrik Skupin (:whimboo) 2011-07-22 05:56:42 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.