Make localization crop tests a bit more useful

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [mozmill-l10n][lib][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 611792 [details] [diff] [review]
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.
Attachment #611792 - Flags: review?(hskupin)
Whiteboard: [mozmill-l10n]
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 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.
Attachment #611792 - Flags: review?(hskupin) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 625632 [details] [diff] [review]
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.
Attachment #611792 - Attachment is obsolete: true
Attachment #625632 - Flags: review?(hskupin)
Blocks: 741301
Blocks: 614579
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.
Attachment #625632 - Flags: review?(hskupin) → review+
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)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → fixed
status-firefox13: --- → fixed
status-firefox14: --- → fixed
status-firefox15: --- → fixed
status-firefox16: --- → fixed
Resolution: --- → FIXED
Blocks: 762796
Whiteboard: [mozmill-l10n] → [mozmill-l10n][lib]
Whiteboard: [mozmill-l10n][lib] → [mozmill-l10n][lib][qa-]
You need to log in before you can comment on or make changes to this bug.