741762 - Make localization crop tests a bit more useful

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Axel Hecht [:Pike]]

Attachment: tolerance and better reporting.

The disabling of the crop tests (bug 741301) scared me, so I dove in and tried to make them better.

Sadly, I still get false positives at least on mac and linux, though different ones. Didn't test win yet.

This patch does a two things: It adds a single pixel tolerance, which fixes the hardly-visible crops to be no-crops. And it changes the reporting to be a bit more telling, I had a hard time to understand the results, so I changed those. Added the crop size, and changed the node descriptions to be closer to CSS, plus always including the localName, which helps to debug a good deal.

I wonder what we can do to get those false positives down, or if there's a way to rather diff the results of a l10n build against en-US than to do nothing. This mostly boils down to en-US not enforcing to not crop. On the mac, I actually think it's a bug, but on other platforms it may not be.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I know how you feel but we can't leave tests enabled now with the CI in-place which are always failing. So good to see that you want to see it fixed. I will have a look at the patch soon.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 611792 [details] [diff] [review]
tolerance and better reporting.

Just as a note in-front, I haven't tested this patch yet. Just looking through the code and trying to understand/remember what we did a year or so ago.

>-  var badRects = [];
>+  var badRects = [], px;

Can you please declare the second variable in a new line and give it a better name? I would like to know what it is when reading its name. Right now it's foo for me.

>   // check width
[..]
>+  if (childBox.height && parentBox.screenX - childBox.screenX > tolerance) {

Would you mind enhancing the comment a bit, so that it describes what we compare here? It's hard to figure out when reading the code. And I assume that it is clear for you at the moment. Also please use a bracket around the subtraction.

>   if (childBox.height && childBox.screenX + childBox.width >
>-      parentBox.screenX + parentBox.width) {
>+      parentBox.screenX + parentBox.width + tolerance) {

Same here as above.

>-    if (childBox.width && childBox.screenY < parentBox.screenY) {
>-      badRects.push([childBox.x, childBox.y, parentBox.y - childBox.y,
>-                     childBox.width]);
>+    if (childBox.width && parentBox.screenY - childBox.screenY > tolerance) {
>+      px = parentBox.y - childBox.y;
>+      badRects.push([childBox.x, childBox.y,
>+                     childBox.width, px]);

So you are changing the order of parameters here. Is that expected?

Otherwise I like that patch. Having the number of pixels which overlap in the failure string makes it way more obvious for localizers.

Comment 3

[Axel Hecht [:Pike]]

Attachment: better comments and compares

Updated the patch to have better comments, I also reordered the comparisons to make them easier to understand.

Sadly, I totally fail to run mozmill tests at all right now, stopped trying after the hour.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 625632 [details] [diff] [review]
better comments and compares

Axel, this patch looks great. Thanks for doing it. I have tested some builds and they all pass. I'm going to land that right away.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Pushed:
http://hg.mozilla.org/qa/mozmill-tests/rev/4004d89b7ab9 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/2525ad415eed (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/ff038ef70047 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/cee038ce7347 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/218dc1fb11d4 (esr10)