Closed Bug 562084 Opened 11 years ago Closed 5 years ago

Tracking bug for Mozmill l10n test-run

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: adriank, Unassigned)

References

()

Details

(Keywords: meta, Whiteboard: [mozmill-l10n][MozmillTestday][mozmill-doc-needed])

Attachments

(2 files, 5 obsolete files)

To have a common ground for the upcoming Mozmill L10n tests and probably accessibility tests too, we'll need to have a L10n shared module.
Depends on: 504635
The basics for walking through the complete DOM should be a separate API which can be shared between at least l10n and accessibility tests.

Regarding bug 504635 we should move getProperty and getEntity over to this API once it has been created.
This should be a meta bug. Please file any of the needed implementations in their own bug.
Keywords: meta
Summary: [shared module] Create L10nApi shared module → [shared module] Tracking bug of Mozmill l10n tests
Summary: [shared module] Tracking bug of Mozmill l10n tests → Tracking bug for Mozmill l10n test-run
Whiteboard: [mozmill-l10n]
Assignee: akalla → nobody
Component: Mozmill → Mozmill Tests
QA Contact: mozmill → mozmilltests
Whiteboard: [mozmill-l10n] → [mozmill-automation][mozmill-l10n]
Depends on: 587819
included: both shared-modules and both preferences tests (cropped
elements + duplicated access keys).

What does NOT work, *yet* (aka "Known issues"):
-the sub-tabs in the "Advanced" tab in the preferences window are
being tested as a whole, so may show not existing duplicated access
keys
-the history items in the "Privacy" tab in the preferences window are
being tested as a whole, so may show not existing duplicated access
keys (but duplicated access keys found in the current Polish nightlies
are really there)
-in the preferences window the OK, Cancel and Help buttons (Win/Lin)
are not being treated as part of the preferences panes, so conflicts
with the Help-accesskey are not being detected (e.g. such conflict
exists in current Polish nightlies). One reason for that is also, that
I currently ignore nodes with accesskeys but without IDs
-in the "Application"-tab the test for cropped elements finds a
cropped element which probably isn't really cropped - will have to
investigate it further
-no screenshots for accesskeys tests are being taken
-currently testing just one window opened from the preferences panes -
still have to add the rest of them
-everything was tested just on Mac. I expect some problems, that will
have to be solved, on Windows - due to the use of modal windows there
(and due to the place where I save the screenshots - "/tmp/")

What has to be changed:
-instead of using the "jumlib.assert()" use "standard" Mozmill assert functions

What has to be added:
-tests for the update-window
Attachment #477290 - Flags: feedback?(hskupin)
(In reply to comment #3)
> included: both shared-modules and both preferences tests (cropped
> elements + duplicated access keys).

Please separate those into API's and tests as discussed in the meeting.

> -the sub-tabs in the "Advanced" tab in the preferences window are
> being tested as a whole, so may show not existing duplicated access
> keys

What is blocking us to iterate through the tabs?

> -the history items in the "Privacy" tab in the preferences window are
> being tested as a whole, so may show not existing duplicated access
> keys (but duplicated access keys found in the current Polish nightlies
> are really there)

Is that a problem with the dropdown which let the user select the history mode? Means we can't detect when such a node changes panes? Or do we have to? Aren't those three panes (or whatever type it is) existent all the time?

> -in the preferences window the OK, Cancel and Help buttons (Win/Lin)
> are not being treated as part of the preferences panes, so conflicts
> with the Help-accesskey are not being detected (e.g. such conflict
> exists in current Polish nightlies). One reason for that is also, that
> I currently ignore nodes with accesskeys but without IDs

So one big question before glancing at the code... What about deep hierarchies? When a top level element has an accesskey assigned and an element in i.e. the 2nd tab of the advanced pane has the same accesskey, the test will catch that?

> -no screenshots for accesskeys tests are being taken

That's not dramatic. It's not part of your current work. We only have to make sure that it will be possible in the future.

> -instead of using the "jumlib.assert()" use "standard" Mozmill assert functions

No, we don't want to do that. The complete test should be run and not stop when the first duplicate accesskey has been found. Jumlib is the only way at the moment.

> What has to be added:
> -tests for the update-window

Don't start with those yet. Bug 596813 will completely change everything.
PS: Accesskeys in the pref dialog are broken at large, at least I guess they still are. It's good enough to just make them work on the current pane, and just live with the fact that they're going to be conflicting cross pane. That's leaving us with unpredictable behaviour, depending on which panes you opened in the current pref window, but that's what it is in en-US, too.
OK, that was probably hard to get:

- all loaded xul content is looked at for accesskeys.

So the question is not whether another prefpane is visible, but if it was loaded.

If you load all panes, you'll have accesskey conflicts, already in en-US.

It's good enough if the accesskeys work after close and reopening the prefwindow, as then there's only the visible pane loaded.
Comment on attachment 477290 [details] [diff] [review]
WIP: L10n shared-modules + preferences tests

>+var teardownModule = function(module) {
>+  PrefsAPI.openPreferencesDialog(prefPaneReset);
>+}

We shouldn't execute any ui interaction in teardownModule. I wouldn't obey the resetting of the preferences dialog for now. 

>+var prefPanesTest = function(controller) {

Please put any helper function below the main test function. That makes it easier to follow the test.

>+  var ids = [
>+               { id : "paneMain", type : "prefpane", dialog : prefDialog}, // the first pane shown after opening the prefwindow has not the status "selected", thus is being ignored - a Firefox bug (??)

Why do you need selected? Check my preferencesDialog API how we have solved it.

[..]
>+               { id : "paneMain", type : "prefpane", dialog : prefDialog},

Again?

>+               { id : "popupPolicyButton", type : "window", title : popuppermissionstitle, parentId : "paneContent"},

Instead of having a flat list, what would stop us from using a deep hierarchy? 

>+  // prevent trouble
>+  prefDialog.paneId = "paneMain";
>+  controller.sleep(gDelay);

For the final version we have to remove all sleep calls.

>+  domWalker.walk(controller, root, l10n.filterAccessKeys, l10n.callbackAccessKeys, l10n.checkWindowAccessKeys, true, ids);

For future endurance we should use a JSON object as parameter. It would also make the separate parameters better understandable.

>+++ b/shared-modules/testDOMUtilitiesAPI.js

>+  // filter out non-window ids
>+  getWindowIdSets : function DOMWalker_getWindowIdSets(ids, parentId) {

Please give functions a better documentation for what they are used for.

>+    var windowIds = [];
>+    for (var i = 0; i < ids.length; i++) {
>+        if (ids[i].type == 'window' && ids[i].parentId == parentId) {

Instead of type you probably want to use target? The popupPolicyButton from the tests is a button and not a window. This can be misleading.

>+            windowIds.push(ids[i]);
>+        }

Please use 2 blanks for indentation everywhere and limit your code to 80 chars per line.

>+  prepareNewDialog : function DOMWalker_prepareNewDialog(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {

Why is the cont (controller) the last parameter for this and the following functions? As long as we are using a list of parameters please don't switch the order. Keep it similar to the domWalker.walk definition. Also 'prepareNewDialog' isn't a name which tells me what this function is used for.

>+  prepareNewPrefPane : function DOMWalker_prepareNewPrefPane(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {
>+  prepareNewWindow : function DOMWalker_prepareNewWindow(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {
[..]
>+  prepareNewWizardPage : function DOMWalker_prepareNewWizardPage(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {

Why can't we get rid of all those functions and simply have the code inside the prepareNewDialog (or whatever it will be called) function with a setup step, the walk call, and a teardown step.

>+    let newWindow = mozmill.utils.getWindowByTitle(idSet.title);
>+    let newController = new mozmill.controller.MozMillController(newWindow);
>+    newController.sleep(1000);

Don't do that! Use UtilsAPI.windowHelper().

>+  walk : function DOMWalker_walk(cont, root, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, main, ids) {
>+    var doc = cont.window.document;
>+    var badBoxes = [];

Please use a better naming convention for you variables. Which boxes are bad?

>+    var nodes = [];
>+    
>+    if (root.hasChildNodes()) {
>+      nodes = root.childNodes;
>+
>+      for (var i = 0; i < nodes.length; i++) {

Why do you declare nodes outside of the if body? Why you need it at all? Use for each to iterate through child nodes.

>+        var wanted = callbackFilterNodes(nodes[i]);

s/wanted/result

>+        if (wanted == FILTER_ACCEPT) {
>+            var badBox = callbackForAcceptedNode(nodes[i]);
>+            if (badBox != undefined && badBox.length > 0) {

badBox is an array. So please reflect that in the name. And why does callbackForAcceptedNode return a bad box? 

>+    if (main == true) {

You should probably use an internal function which will be called recursively or check the functions callee, which should also work.

>+        if (ids != null && ids.length > 0) {
>+            for (var i = 0; i < ids.length; i++) {
>+                var element = doc.getElementById(ids[i].id);
>+                if (element != undefined && element != null) {
>+                    this.prepareNewDialog(ids[i], callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont)
>+                }
>+            }
>+        }

Please please add comments! For what is that code for?

>+l10n.prototype = {
>+  
>+  callbackAccessKeys : function l10n_callbackAccessKeys(node) {
>+    return this.l10n.checkAccessKey(node);
>+  },

Any reason why you are using 'this.l10n'? this is the instance of the l10n class. Also why is the code in another function? It also applies to all other callbacks.

>+  checkAccessKey : function l10n_checkAccessKey(node) {

Check the accesskey for what?

>+    var aKeysList = [];
>+    if (node.accessKey && node.id) {
>+      var accessKey = node.accessKey.toLowerCase();
>+      aKeysList.push([accessKey, node.id]);
>+    }
>+    return aKeysList;

As discussed in the meeting returning the reference to the object instead of the id only, should be the better solution. 

Other code (callbacks and helper functions) I haven't checked for now because it is really hard to understand most of it without any comment. Please do me a favor and have it ready for the next feedback/review.
Attachment #477290 - Flags: feedback?(hskupin) → feedback-
(In reply to comment #6)
> - all loaded xul content is looked at for accesskeys.

Really? Hasn't this been fixed with bug 143065 a while back?
(In reply to comment #4)
> (In reply to comment #3)
> > included: both shared-modules and both preferences tests (cropped
> > elements + duplicated access keys).
> 
> Please separate those into API's and tests as discussed in the meeting.


(...............)


> > -the sub-tabs in the "Advanced" tab in the preferences window are
> > being tested as a whole, so may show not existing duplicated access
> > keys
> 
> What is blocking us to iterate through the tabs?

Every sub-tab has an own access key scope. So if I test everything in that prefpane, I test a few scopes as one, so I detect access key conflicts that are just not there.
So I have to filter out the not-selected tabs before testing the pane, switch tabs, test again, and so on.


> > -the history items in the "Privacy" tab in the preferences window are
> > being tested as a whole, so may show not existing duplicated access
> > keys (but duplicated access keys found in the current Polish nightlies
> > are really there)
> 
> Is that a problem with the dropdown which let the user select the history mode?
> Means we can't detect when such a node changes panes? Or do we have to? Aren't
> those three panes (or whatever type it is) existent all the time?

Each one of the tree panes seems to be in an own access key scope, so I have to test them separately, like with the advanced prefpane.

 
> > -in the preferences window the OK, Cancel and Help buttons (Win/Lin)
> > are not being treated as part of the preferences panes, so conflicts
> > with the Help-accesskey are not being detected (e.g. such conflict
> > exists in current Polish nightlies). One reason for that is also, that
> > I currently ignore nodes with accesskeys but without IDs
> 
> So one big question before glancing at the code... What about deep hierarchies?
> When a top level element has an accesskey assigned and an element in i.e. the
> 2nd tab of the advanced pane has the same accesskey, the test will catch that?

We have to filter out everything which is not in one access key scope before testing it through the filter-function. Then it will work correctly.

 
> > -no screenshots for accesskeys tests are being taken
> 
> That's not dramatic. It's not part of your current work. We only have to make
> sure that it will be possible in the future.

It's not hard to do - I just didn't plan to take screenshots for the access keys tests and have to change that now.

 
> > -instead of using the "jumlib.assert()" use "standard" Mozmill assert functions
> 
> No, we don't want to do that. The complete test should be run and not stop when
> the first duplicate accesskey has been found. Jumlib is the only way at the
> moment.

Ok, didn't know that.


> > What has to be added:
> > -tests for the update-window
> 
> Don't start with those yet. Bug 596813 will completely change everything.

Thanks. I'd have wasted a lot of time if I didn't know it :)



(In reply to comment #7)
> Comment on attachment 477290 [details] [diff] [review]
> WIP: L10n shared-modules + preferences tests
> 
> >+var teardownModule = function(module) {
> >+  PrefsAPI.openPreferencesDialog(prefPaneReset);
> >+}
> 
> We shouldn't execute any ui interaction in teardownModule. I wouldn't obey the
> resetting of the preferences dialog for now.

ok


> >+var prefPanesTest = function(controller) {
> 
> Please put any helper function below the main test function. That makes it
> easier to follow the test.

ok. Just wanted to have them alphabetically.

 
> >+  var ids = [
> >+               { id : "paneMain", type : "prefpane", dialog : prefDialog}, // the first pane shown after opening the prefwindow has not the status "selected", thus is being ignored - a Firefox bug (??)
> 
> Why do you need selected? Check my preferencesDialog API how we have solved it.

In fact, I'm using exactly that API for switching the panes 

> [..]
> >+               { id : "paneMain", type : "prefpane", dialog : prefDialog},
> 
> Again?

The first time it wasn't selected, so it was filtered out.

 
> >+               { id : "popupPolicyButton", type : "window", title : popuppermissionstitle, parentId : "paneContent"},
> 
> Instead of having a flat list, what would stop us from using a deep hierarchy?

That's a good idea.

 
> >+  // prevent trouble
> >+  prefDialog.paneId = "paneMain";
> >+  controller.sleep(gDelay);
> 
> For the final version we have to remove all sleep calls.

right


> >+  domWalker.walk(controller, root, l10n.filterAccessKeys, l10n.callbackAccessKeys, l10n.checkWindowAccessKeys, true, ids);
> 
> For future endurance we should use a JSON object as parameter. It would also
> make the separate parameters better understandable.

ok


> >+++ b/shared-modules/testDOMUtilitiesAPI.js
> 
> >+  // filter out non-window ids
> >+  getWindowIdSets : function DOMWalker_getWindowIdSets(ids, parentId) {
> 
> Please give functions a better documentation for what they are used for.

will be done


> >+    var windowIds = [];
> >+    for (var i = 0; i < ids.length; i++) {
> >+        if (ids[i].type == 'window' && ids[i].parentId == parentId) {
> 
> Instead of type you probably want to use target? The popupPolicyButton from the
> tests is a button and not a window. This can be misleading.

right, will change that

 
> >+            windowIds.push(ids[i]);
> >+        }
> 
> Please use 2 blanks for indentation everywhere and limit your code to 80 chars
> per line.

ok


> >+  prepareNewDialog : function DOMWalker_prepareNewDialog(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {
> 
> Why is the cont (controller) the last parameter for this and the following
> functions? As long as we are using a list of parameters please don't switch the
> order. Keep it similar to the domWalker.walk definition.

Originally, I didn't plan to have a controller object there but had to change that and that's why it landed as the last one. Will be changed.


>  Also
> 'prepareNewDialog' isn't a name which tells me what this function is used for.

Still looking for a good name here. For now, it'll be "openNew"

 
> >+  prepareNewPrefPane : function DOMWalker_prepareNewPrefPane(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {
> >+  prepareNewWindow : function DOMWalker_prepareNewWindow(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {
> [..]
> >+  prepareNewWizardPage : function DOMWalker_prepareNewWizardPage(idSet, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont) {
> 
> Why can't we get rid of all those functions and simply have the code inside the
> prepareNewDialog (or whatever it will be called) function with a setup step,
> the walk call, and a teardown step.

I wanted them separated to avoid blown-up functions.

 
> >+    let newWindow = mozmill.utils.getWindowByTitle(idSet.title);
> >+    let newController = new mozmill.controller.MozMillController(newWindow);
> >+    newController.sleep(1000);
> 
> Don't do that! Use UtilsAPI.windowHelper().

It's hard to use an undocumented function ( https://developer.mozilla.org/en/Mozmill_Tests/Shared_Modules/UtilsAPI ), when you don't know about it...


> >+  walk : function DOMWalker_walk(cont, root, callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, main, ids) {
> >+    var doc = cont.window.document;
> >+    var badBoxes = [];
> 
> Please use a better naming convention for you variables. Which boxes are bad?

The ones which need later attention, e.g. with coordinates of cropped elements or duplicated access keys.
Renamed to "results".


> >+    var nodes = [];
> >+    
> >+    if (root.hasChildNodes()) {
> >+      nodes = root.childNodes;
> >+
> >+      for (var i = 0; i < nodes.length; i++) {
> 
> Why do you declare nodes outside of the if body? Why you need it at all? Use
> for each to iterate through child nodes.

It could have been done better, so I tweaked it a bit.
"For each" cannot be used here, as it is an array. MDC warns about using for each for arrays too: https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...in#Description


> >+        var wanted = callbackFilterNodes(nodes[i]);
> 
> s/wanted/result

Renamed to "nodeStatus", as result is being used for something else now (see below).

 
> >+        if (wanted == FILTER_ACCEPT) {
> >+            var badBox = callbackForAcceptedNode(nodes[i]);
> >+            if (badBox != undefined && badBox.length > 0) {
> 
> badBox is an array. So please reflect that in the name. And why does
> callbackForAcceptedNode return a bad box?

I'm not good in naming variables... The full name would be:
callbackForNodeAcceptedForTestingReturningAnArrayWithWhateverYouWant
It's "result" now.


> >+    if (main == true) {
> 
> You should probably use an internal function which will be called recursively
> or check the functions callee, which should also work.

Function.caller does not return anything usefull here, as does arguments.callee...


> >+        if (ids != null && ids.length > 0) {
> >+            for (var i = 0; i < ids.length; i++) {
> >+                var element = doc.getElementById(ids[i].id);
> >+                if (element != undefined && element != null) {
> >+                    this.prepareNewDialog(ids[i], callbackFilterNodes, callbackForAcceptedNode, callbackProcessResults, ids, cont)
> >+                }
> >+            }
> >+        }
> 
> Please please add comments! For what is that code for?

Comments are on the way.
The code is for:
1) Finding out if we want to open something
2) Finding the "trigger"-node for opening something
3) Call preparenewDialog

Moved the for loop to DOMWalker.openNew .


> >+l10n.prototype = {
> >+  
> >+  callbackAccessKeys : function l10n_callbackAccessKeys(node) {
> >+    return this.l10n.checkAccessKey(node);
> >+  },
> 
> Any reason why you are using 'this.l10n'? this is the instance of the l10n
> class.

Because this.checkAccessKey(node) didn't want to work anyhow.


> Also why is the code in another function? It also applies to all other
> callbacks.

I probably wanted too much separation...


> >+  checkAccessKey : function l10n_checkAccessKey(node) {
> 
> Check the accesskey for what?
> 
> >+    var aKeysList = [];
> >+    if (node.accessKey && node.id) {
> >+      var accessKey = node.accessKey.toLowerCase();
> >+      aKeysList.push([accessKey, node.id]);
> >+    }
> >+    return aKeysList;
> 
> As discussed in the meeting returning the reference to the object instead of
> the id only, should be the better solution. 

Will be done - just didn't have enough time between the meeting and uploading that patch to work on this.


> Other code (callbacks and helper functions) I haven't checked for now because
> it is really hard to understand most of it without any comment. Please do me a
> favor and have it ready for the next feedback/review.

Yep, it will be done.





(In reply to comment #8)
> (In reply to comment #6)
> > - all loaded xul content is looked at for accesskeys.
> 
> Really? Hasn't this been fixed with bug 143065 a while back?

Yes, that's definitely fixed (at least on trunk).
I'll update the patch here too, but later today.

Here the updated TODO list (all items that are gone, are fixed):

What does NOT work, *yet* (aka "Known issues"):
-in the "Application"-tab the test for cropped elements finds a
cropped element which probably isn't really cropped. This does not happen in some locales, like in "DE" - no error is reported. Because of that, I'm not sure if it is a bug in en-US Firefox, or if I have to tweak the checkDimensions method...
-currently testing just one window opened from the preferences panes -
still have to add the rest of them
-everything was tested just on Mac. I expect some problems, that will
have to be solved, on Windows - due to the use of modal windows there

What has to be added:
-tests for the update-window
> Every sub-tab has an own access key scope. So if I test everything in that
> prefpane, I test a few scopes as one, so I detect access key conflicts that are
> just not there.
> So I have to filter out the not-selected tabs before testing the pane, switch
> tabs, test again, and so on.

Does it mean you have to run tests against the same pane multiple times but only with a different tab selected? If that's the case it will put us into a state where we cannot let the DOM walker run against a top level element and it's doing everything automatically. If I get it right, sub-calls of the DOM walker don't know anything about the information collected by higher-level tests?

> Each one of the tree panes seems to be in an own access key scope, so I have to
> test them separately, like with the advanced prefpane.

Which type of element are those panes? In the above case there is no tab element the user can use to select a specific pane, but a menulist is doing that instead. Do we know from the pane node which element is modifying its selectedIndex property? I think we don't, right? 

> > So one big question before glancing at the code... What about deep hierarchies?
> > When a top level element has an accesskey assigned and an element in i.e. the
> > 2nd tab of the advanced pane has the same accesskey, the test will catch that?
> 
> We have to filter out everything which is not in one access key scope before
> testing it through the filter-function. Then it will work correctly.

What's the algorithm for that? As far as I was able to see you simply collect a list of nodes.

> > >+  var ids = [
> > >+               { id : "paneMain", type : "prefpane", dialog : prefDialog}, // the first pane shown after opening the prefwindow has not the status "selected", thus is being ignored - a Firefox bug (??)
> > 
> > Why do you need selected? Check my preferencesDialog API how we have solved it.
> 
> In fact, I'm using exactly that API for switching the panes 

So you say, when opening the preferences dialog the selected pane is not shown correctly?

> > >+               { id : "paneMain", type : "prefpane", dialog : prefDialog},
> > 
> > Again?
> 
> The first time it wasn't selected, so it was filtered out.

So remove the first instance from the array?

> > Why can't we get rid of all those functions and simply have the code inside the
> > prepareNewDialog (or whatever it will be called) function with a setup step,
> > the walk call, and a teardown step.
> 
> I wanted them separated to avoid blown-up functions.

Lets revise later once I got a better understanding of the code.

> It's hard to use an undocumented function (
> https://developer.mozilla.org/en/Mozmill_Tests/Shared_Modules/UtilsAPI ), when
> you don't know about it...

We are lacking some docs, I know. It's something which will (hopefully) change next quarter with automatically generated docs. Until then check our existing tests or simply ask. I'm around mostly all the time.

> > >+    var nodes = [];
> > >+    
> > >+    if (root.hasChildNodes()) {
> > >+      nodes = root.childNodes;
> > >+
> > >+      for (var i = 0; i < nodes.length; i++) {
> > 
> > Why do you declare nodes outside of the if body? Why you need it at all? Use
> > for each to iterate through child nodes.
> 
> It could have been done better, so I tweaked it a bit.
> "For each" cannot be used here, as it is an array. MDC warns about using for
> each for arrays too:
> https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...in#Description

Thanks for that information. I haven't known that. But AFAIR nodes is only used inside the if clause. So move its declaration directly to the assignment.

> > badBox is an array. So please reflect that in the name. And why does
> > callbackForAcceptedNode return a bad box?
> 
> I'm not good in naming variables... The full name would be:
> callbackForNodeAcceptedForTestingReturningAnArrayWithWhateverYouWant
> It's "result" now.

It's still an array. So result would not match. But lets push this to a later time.

> > >+    if (main == true) {
> > 
> > You should probably use an internal function which will be called recursively
> > or check the functions callee, which should also work.
> 
> Function.caller does not return anything usefull here, as does
> arguments.callee...

I'm not sure right now if this is the right way to go. We should cover that in our 1-1.

> > >+l10n.prototype = {
> > >+  
> > >+  callbackAccessKeys : function l10n_callbackAccessKeys(node) {
> > >+    return this.l10n.checkAccessKey(node);
> > >+  },
> > 
> > Any reason why you are using 'this.l10n'? this is the instance of the l10n
> > class.
> 
> Because this.checkAccessKey(node) didn't want to work anyhow.

From inside the DOM walker? You will have to create an instance of the l10n class first.

> > > - all loaded xul content is looked at for accesskeys.
> > 
> > Really? Hasn't this been fixed with bug 143065 a while back?
> 
> Yes, that's definitely fixed (at least on trunk).

Good to know!
Reading through the patch, I guess we'll need to have a at least a minX for the screenshot, too, for RTL locales overflowing to the left.
Depends on: 604411
Attached patch WIP: L10n preferences tests (obsolete) — Splinter Review
updated preferences tests to work with the in-review shared modules
Attachment #477290 - Attachment is obsolete: true
Attached patch WIP: l10n preferences tests (obsolete) — Splinter Review
updated to work with the current shard modules patches
Attachment #483197 - Attachment is obsolete: true
Comment on attachment 490494 [details] [diff] [review]
WIP: l10n preferences tests

>+++ b/firefox/testLocalization/testPreferences.js

Localization tests will not be part of the general test-run. Therefore please don't use 'test' as prefix for the folder name.

>+ * The Initial Developer of the Original Code is Mozilla Foundation.

"the Mozilla Foundation"

>+const DELAY = 100;
>+const TIMEOUT = 5000;

I think those can be removed.

>+var jumlib = {}; 
>+Components.utils.import("resource://mozmill/modules/jum.js", jumlib);

>+var setupModule = function(module) {

Please always used named functions.

>+  // getting the preferences controller opens the preferences window on Windows,
>+  // but not on unix
>+  if (mozmill.isWindows)
>+    prefs.openPreferencesDialog(prefPanesTest);
>+  else
>+    controller = mozmill.getPreferencesController();

Why this should not work? None of our tests experience this problem. Also prefPanesTest is not defined. That's why the test doesn't work on Windows. Don't open the window inside of setupModule.

>+var teardownModule = function(module) {
>+  prefs.openPreferencesDialog(prefPaneReset);

A comment why we have to reopen it again would be nice.

>+  var ids =
>+  [
>+    {
>+      attribute : "id",
>+      id : "paneMain",
>+      target : WINDOW_CURRENT,
>+      windowHandler : prefDialog,
>+      subContent :

Please check our formatting style how to format blocks as best as possible:
https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Test_Modules_Refactor

>+var prefPaneReset = function(controller)
>+{
>+  var prefDialog = new prefs.preferencesDialog(controller);
>+
>+  prefDialog.paneId = 'paneMain';
>+  
>+  controller.sleep(DELAY);

Is a delay really necessary?

>+var prefPanesAccessKeyTest = function(controller) {
>+var prefPanesCroppedTest = function(controller) {

Those should be two independent test files. 

>+var testPrefWindowAccessKeys = function() {
>+  // Mac OSX does not support access keys
>+  //if (!mozmill.isMac) {

Even those are not visible we could test them, right?

>+var testPrefPaneReset = function(module) {
>+  prefs.openPreferencesDialog(prefPaneReset);

Please don't do things like that. This is not a test and should be part of setupTest at least.

>+var testPrefWindowCroppedElements = function() {
>+  if (!mozmill.isWindows)
>+    prefs.openPreferencesDialog(prefPanesCroppedTest);

I think this exception does exist because of the problems you have mentioned above?
Attachment #490494 - Flags: review-
Depends on: 612687
Move of Mozmill related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.
Component: Mozmill Tests → Mozmill Automation
Product: Testing → Mozilla QA
QA Contact: mozmill-tests → mozmill-automation
Whiteboard: [mozmill-automation][mozmill-l10n]
Whiteboard: [mozmill-l10n]
Attached patch L10n Preferences Tests v1 (obsolete) — Splinter Review
(In reply to comment #15)
> Comment on attachment 490494 [details] [diff] [review]
> WIP: l10n preferences tests
> 
> >+++ b/firefox/testLocalization/testPreferences.js
> 
> Localization tests will not be part of the general test-run. Therefore please
> don't use 'test' as prefix for the folder name.

ok. Changed to "localizationTests"

> >+ * The Initial Developer of the Original Code is Mozilla Foundation.
> 
> "the Mozilla Foundation"

done

> >+const DELAY = 100;
> >+const TIMEOUT = 5000;
> 
> I think those can be removed.

yep, removed

> >+var jumlib = {}; 
> >+Components.utils.import("resource://mozmill/modules/jum.js", jumlib);
> 
> >+var setupModule = function(module) {
> 
> Please always used named functions.

done

> >+  // getting the preferences controller opens the preferences window on Windows,
> >+  // but not on unix
> >+  if (mozmill.isWindows)
> >+    prefs.openPreferencesDialog(prefPanesTest);
> >+  else
> >+    controller = mozmill.getPreferencesController();
> 
> Why this should not work? None of our tests experience this problem. Also
> prefPanesTest is not defined. That's why the test doesn't work on Windows.
> Don't open the window inside of setupModule.

that was a big mess... Fixed.

> >+var teardownModule = function(module) {
> >+  prefs.openPreferencesDialog(prefPaneReset);
> 
> A comment why we have to reopen it again would be nice.

done

> >+  var ids =
> >+  [
> >+    {
> >+      attribute : "id",
> >+      id : "paneMain",
> >+      target : WINDOW_CURRENT,
> >+      windowHandler : prefDialog,
> >+      subContent :
> 
> Please check our formatting style how to format blocks as best as possible:
> https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Test_Modules_Refactor

done - hopefully correctly. But to be honest: the previous way it seemed much better readable for me...

> >+var prefPaneReset = function(controller)
> >+{
> >+  var prefDialog = new prefs.preferencesDialog(controller);
> >+
> >+  prefDialog.paneId = 'paneMain';
> >+  
> >+  controller.sleep(DELAY);
> 
> Is a delay really necessary?

I used it as it was in testSwitchPanes.js too. Removed now - seems to work without the delay too.

> >+var prefPanesAccessKeyTest = function(controller) {
> >+var prefPanesCroppedTest = function(controller) {
> 
> Those should be two independent test files. 

reverted to independent tests

> >+var testPrefWindowAccessKeys = function() {
> >+  // Mac OSX does not support access keys
> >+  //if (!mozmill.isMac) {
> 
> Even those are not visible we could test them, right?

Yes, we can test them. But I'm not sure if it makes sense, as they are unusuable on a Mac anyway...

> >+var testPrefPaneReset = function(module) {
> >+  prefs.openPreferencesDialog(prefPaneReset);
> 
> Please don't do things like that. This is not a test and should be part of
> setupTest at least.

removed

> >+var testPrefWindowCroppedElements = function() {
> >+  if (!mozmill.isWindows)
> >+    prefs.openPreferencesDialog(prefPanesCroppedTest);
> 
> I think this exception does exist because of the problems you have mentioned
> above?

yep, fixed



On a different topic: I couldn't find a way to "export" the "ids" object to an external file:
-saving it as JSON is not possible, as we have functions there (e.g. the "waitFunction"): https://developer.mozilla.org/en/JSON (chapter: "Limitations")
-the fact that we have instances there, like "prefDialog", does not make it easier.
If there is a working way to do this, I'll be glad to hear about it :)
Attachment #490494 - Attachment is obsolete: true
Attachment #491816 - Flags: review?(hskupin)
Comment on attachment 491816 [details] [diff] [review]
L10n Preferences Tests v1

>+++ b/firefox/localizationTests/testAccessKeysInPreferences.js

Would you have any complains against l10nTests?

>+function setupModule(module) {
>+  controller = mozmill.getBrowserController();
>+  GET_BY_ID = domUtils.DOMWalker.GET_BY_ID;
>+  GET_BY_SELECTOR = domUtils.DOMWalker.GET_BY_SELECTOR;
>+  WINDOW_CURRENT = domUtils.DOMWalker.WINDOW_CURRENT;
>+  WINDOW_MODAL= domUtils.DOMWalker.WINDOW_MODAL;
>+  WINDOW_NEW = domUtils.DOMWalker.WINDOW_NEW;

Those should be declared globally in this file.

>+function teardownModule(module) {
>+  // We have to reset the pane and tabs, so the next test does not test them
>+  // twice (once, when opening the pane at the beginning and the second time,
>+  // when going there because of a subContent entry).
>+  prefs.openPreferencesDialog(prefPaneReset);

What do you mean with next test? cutoff vs. access keys? If that's the case you should think about to implement both tests as restart tests. That way you will really have a clean ui. IMO it would make more sense.

>+  var ids = [
>+    { getBy : GET_BY_ID,
>+      id : "paneMain",
>+      target : WINDOW_CURRENT,
>+      windowHandler : prefDialog,
[..]

>+    { getBy : GET_BY_ID,
>+      id : "paneSync",
>+      target : WINDOW_CURRENT,
>+      windowHandler : prefDialog}
>+  ];

This is list is duplicated across both tests. As given in my last review and in our 1-1 it should be moved out to a json file, which you can read in inside setupModule.

>+function prefPanesAccessKeyTest(controller) {
>+  
>+  // Path to save screenshots under
>+  var path = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
>+                 utils.appInfo.version + "." + utils.appInfo.buildID + "." +
>+                 utils.appInfo.os + ".png";
>+  if (mozmill.isWindows) {
>+    path = "C:\\" + path;
>+  } else {
>+    path = "/tmp/" + path;
>+  }

Given by the other reviews we should move this logic into the screenshot module itself and use it as fallback, if no path is given by the persisted object.

>+function testPrefWindowAccessKeys() {
>+  // Mac OSX does not support access keys
>+  //if (!mozmill.isMac) {
>+    prefs.openPreferencesDialog(prefPanesAccessKeyTest);
>+  //}
>+}

I would still leave it in. The test can still test the access keys, even when those are not visible.

Same comments apply for the cropped tests.
Attachment #491816 - Flags: review?(hskupin) → review-
(In reply to comment #18)
> Comment on attachment 491816 [details] [diff] [review]
> L10n Preferences Tests v1
> 
> >+++ b/firefox/localizationTests/testAccessKeysInPreferences.js
> 
> Would you have any complains against l10nTests?

no

> >+function setupModule(module) {
> >+  controller = mozmill.getBrowserController();
> >+  GET_BY_ID = domUtils.DOMWalker.GET_BY_ID;
> >+  GET_BY_SELECTOR = domUtils.DOMWalker.GET_BY_SELECTOR;
> >+  WINDOW_CURRENT = domUtils.DOMWalker.WINDOW_CURRENT;
> >+  WINDOW_MODAL= domUtils.DOMWalker.WINDOW_MODAL;
> >+  WINDOW_NEW = domUtils.DOMWalker.WINDOW_NEW;
> 
> Those should be declared globally in this file.

ok, will move that

> >+function teardownModule(module) {
> >+  // We have to reset the pane and tabs, so the next test does not test them
> >+  // twice (once, when opening the pane at the beginning and the second time,
> >+  // when going there because of a subContent entry).
> >+  prefs.openPreferencesDialog(prefPaneReset);
> 
> What do you mean with next test? cutoff vs. access keys? If that's the case you
> should think about to implement both tests as restart tests. That way you will
> really have a clean ui. IMO it would make more sense.

that's my opinion too. But do restart tests work correctly now? I remember, that some time ago there where problems with them...

> >+  var ids = [
> >+    { getBy : GET_BY_ID,
> >+      id : "paneMain",
> >+      target : WINDOW_CURRENT,
> >+      windowHandler : prefDialog,
> [..]
> 
> >+    { getBy : GET_BY_ID,
> >+      id : "paneSync",
> >+      target : WINDOW_CURRENT,
> >+      windowHandler : prefDialog}
> >+  ];
> 
> This is list is duplicated across both tests. As given in my last review and in
> our 1-1 it should be moved out to a json file, which you can read in inside
> setupModule.

Let me cite myself:
/citation-start
On a different topic: I couldn't find a way to "export" the "ids" object to an
external file:
-saving it as JSON is not possible, as we have functions there (e.g. the
"waitFunction"): https://developer.mozilla.org/en/JSON (chapter: "Limitations")
-the fact that we have instances there, like "prefDialog", does not make it
easier.
If there is a working way to do this, I'll be glad to hear about it :)
/citation-end
Because of those two points, I couldn't get it work...
 
> >+function prefPanesAccessKeyTest(controller) {
> >+  
> >+  // Path to save screenshots under
> >+  var path = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
> >+                 utils.appInfo.version + "." + utils.appInfo.buildID + "." +
> >+                 utils.appInfo.os + ".png";
> >+  if (mozmill.isWindows) {
> >+    path = "C:\\" + path;
> >+  } else {
> >+    path = "/tmp/" + path;
> >+  }
> 
> Given by the other reviews we should move this logic into the screenshot module
> itself and use it as fallback, if no path is given by the persisted object.

ok

> >+function testPrefWindowAccessKeys() {
> >+  // Mac OSX does not support access keys
> >+  //if (!mozmill.isMac) {
> >+    prefs.openPreferencesDialog(prefPanesAccessKeyTest);
> >+  //}
> >+}
> 
> I would still leave it in. The test can still test the access keys, even when
> those are not visible.

ok, so I'll remove the commented out code.
 
> Same comments apply for the cropped tests.

ok
(In reply to comment #19)
> that's my opinion too. But do restart tests work correctly now? I remember,
> that some time ago there where problems with them...

I'm not aware of any problems. Which ones do you remember?

> On a different topic: I couldn't find a way to "export" the "ids" object to an
> external file:
> -saving it as JSON is not possible, as we have functions there (e.g. the
> "waitFunction"): https://developer.mozilla.org/en/JSON (chapter: "Limitations")
> -the fact that we have instances there, like "prefDialog", does not make it
> easier.
> If there is a working way to do this, I'll be glad to hear about it :)

Sorry, missed that comment. Please file a new bug right away so we can work on that part after the landing. I have ideas...
Attached patch L10n Preferences Tests v2 (obsolete) — Splinter Review
(In reply to comment #20)
> (In reply to comment #19)
> > that's my opinion too. But do restart tests work correctly now? I remember,
> > that some time ago there where problems with them...
> 
> I'm not aware of any problems. Which ones do you remember?

ok, I guess I did just remember something wrong ;)

> > On a different topic: I couldn't find a way to "export" the "ids" object to an
> > external file:
> > -saving it as JSON is not possible, as we have functions there (e.g. the
> > "waitFunction"): https://developer.mozilla.org/en/JSON (chapter: "Limitations")
> > -the fact that we have instances there, like "prefDialog", does not make it
> > easier.
> > If there is a working way to do this, I'll be glad to hear about it :)
> 
> Sorry, missed that comment. Please file a new bug right away so we can work on
> that part after the landing. I have ideas...

ok, I'll do that
Attachment #491816 - Attachment is obsolete: true
Attachment #491877 - Flags: review?(hskupin)
Depends on: 613542
Depends on: 613543
Comment on attachment 491877 [details] [diff] [review]
L10n Preferences Tests v2

>+++ b/firefox/l10nTests/testAccessKeys/test1.js

Because we do not have any clue what this test is checking now, there should be a jsdoc section right below the license header which explains what this test is aiming for. 

>+var jumlib = {}; 
>+Components.utils.import("resource://mozmill/modules/jum.js", jumlib);

There is no need to import this module anymore. I can't find any jumlib usage.

Same applies to the other test file.

r=me with the above comments addressed.
Attachment #491877 - Flags: review?(hskupin) → review+
addressed review comments from comment 22
Attachment #491877 - Attachment is obsolete: true
Attachment #492863 - Flags: review?(hskupin)
Attachment #492863 - Flags: review?(hskupin) → review+
Whiteboard: [mozmill-l10n] → [mozmill-l10n][needs landing of bug 587819 and bug 604411]
Depends on: 614924
Comment on attachment 492863 [details] [diff] [review]
L10n Preferences Tests v3 [checked-in]

The preferences tests have been landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/9ece7a033bfe

Adrian, before the check-in I have cleared up all the remaining white-space issues, which were still in all files. Mostly it applies to blank lines. Please make sure to work on the checked-in patches for the backports.
Attachment #492863 - Attachment description: L10n Preferences Tests v3 → L10n Preferences Tests v3 [checked-in]
Whiteboard: [mozmill-l10n][needs landing of bug 587819 and bug 604411] → [mozmill-l10n]
Depends on: 614944
Depends on: 614949
Whiteboard: [mozmill-l10n] → [mozmill-l10n][MozmillTestday]
No longer depends on: 614047
Depends on: 615004
Preferences tests backport to 1.9.2 - fix for Bug 614924 included
Attachment #494075 - Flags: review?(hskupin)
Attachment #494075 - Flags: review?(hskupin) → review+
Comment on attachment 494075 [details] [diff] [review]
L10n Preferences Tests for 1.9.2 [checked-in]

Landed on 1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/cd9d2d438f59
Attachment #494075 - Attachment description: L10n Preferences Tests for 1.9.2 → L10n Preferences Tests for 1.9.2 [checked-in]
Depends on: 621013
Adrian, we absolutely need some documentation on MDN which explains how to create L10n tests, how the idSet object has to look like, and how results can locally be evaluated.
Assignee: nobody → akalla
Whiteboard: [mozmill-l10n][MozmillTestday] → [mozmill-l10n][MozmillTestday][mozmill-doc-needed]
Depends on: 628841
Component: Mozmill Automation → Mozmill Tests
QA Contact: mozmill-automation → mozmill-tests
Depends on: 741301
No longer depends on: 614579
Assignee: akalla → nobody
Status: ASSIGNED → NEW
Calling this fixed given the landing of code in the past.
Status: NEW → RESOLVED
Closed: 5 years ago
QA Contact: hskupin
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.