Closed
Bug 741762
Opened 12 years ago
Closed 12 years ago
Make localization crop tests a bit more useful
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 fixed)
People
(Reporter: Pike, Assigned: Pike)
References
Details
(Whiteboard: [mozmill-l10n][lib][qa-])
Attachments
(1 file, 1 obsolete file)
4.59 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
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+
Comment 5•12 years ago
|
||
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
Closed: 12 years ago
status-firefox-esr10:
--- → fixed
status-firefox13:
--- → fixed
status-firefox14:
--- → fixed
status-firefox15:
--- → fixed
status-firefox16:
--- → fixed
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [mozmill-l10n] → [mozmill-l10n][lib]
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•