Last Comment Bug 741762 - Make localization crop tests a bit more useful
: Make localization crop tests a bit more useful
Status: RESOLVED FIXED
[mozmill-l10n][lib][qa-]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Axel Hecht [:Pike]
:
Mentors:
Depends on:
Blocks: 614579 741301 762796
  Show dependency treegraph
 
Reported: 2012-04-03 06:49 PDT by Axel Hecht [:Pike]
Modified: 2012-08-14 14:57 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
tolerance and better reporting. (3.92 KB, patch)
2012-04-03 06:49 PDT, Axel Hecht [:Pike]
hskupin: review-
Details | Diff | Splinter Review
better comments and compares (4.59 KB, patch)
2012-05-21 06:54 PDT, Axel Hecht [:Pike]
hskupin: review+
Details | Diff | Splinter Review

Description Axel Hecht [:Pike] 2012-04-03 06:49:20 PDT
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.
Comment 1 Henrik Skupin (:whimboo) 2012-04-04 10:40:45 PDT
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) 2012-04-04 10:58:27 PDT
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] 2012-05-21 06:54:56 PDT
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.
Comment 4 Henrik Skupin (:whimboo) 2012-06-07 23:50:19 PDT
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.

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