Closed Opened 5 years ago Closed 5 years ago

# Fix import-tests.py to avoid breaking three multicol tests when importing them

Not set
normal

## Tracking

### ()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

## Attachments

### (1 file)

 58 bytes, text/x-review-board-request dbaron : review+ Details
The fix for this will fix three reftests for multicol related properties:

Fails on Firefox, passes on Chrome->
1. multicol-nested-column-rule-001.xht
2. multicol-rule-samelength-001.xht

Fails on Firefox, fails on Chrome
3. multicol-columns-invalid-002.xht
Summary: Fix import-tests.py to remove --moz prefix from being inserted → Fix import-tests.py to stop --moz prefix from being inserted
Perhaps this should be fixed as part of the bug that introduced the problem rather than creating a separate bug?
Summary: Fix import-tests.py to stop --moz prefix from being inserted → Fix import-tests.py to stop -moz prefix from being inserted in multicolumn tests
Summary: Fix import-tests.py to stop -moz prefix from being inserted in multicolumn tests → Fix import-tests.py to fix parsing errors in three multicol tests
Summary: Fix import-tests.py to fix parsing errors in three multicol tests → Fix import-tests.py so it won't introduce XML parsing errors (from "--") in three multicol tests
(In reply to Neerja:neerja from comment #0)
> The fix for this will fix three reftests for multicol related properties:
>
> Fails on Firefox, passes on Chrome->
> 1. multicol-nested-column-rule-001.xht
> 2. multicol-rule-samelength-001.xht
>
> Fails on Firefox, fails on Chrome->
> 3. multicol-columns-invalid-002.xht

Adding to original description:
The tests mentioned above all fail because of some injections made by import-test.py, EG:

Both multicol-nested-column-rule-001.xht and multicol-columns-invalid-002.xht have invalid string '--moz' being inserted which leads to a parsing failure.

multicol-rule-samelength-001.xht has injections like below that need to be fixed:
-moz--moz-column-rule-color: green;
-moz--moz-column-rule-style: solid;
-moz--moz-column-rule-width: 5em;
Summary: Fix import-tests.py so it won't introduce XML parsing errors (from "--") in three multicol tests → Fix import-tests.py to fix three failing multicol tests
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #1)
> Perhaps this should be fixed as part of the bug that introduced the problem
> rather than creating a separate bug?

Sorry about the ambiguous bug title and description before. This is a completely new bug, added more details to the bug description and changed the title to make it clear.
No longer blocks: 698783
Blocks: 1295261
Summary: Fix import-tests.py to fix three failing multicol tests → Fix import-tests.py to avoid breaking three multicol tests when importing them
[sorry for me & dholbert editing the summary so many times; I think we've got the scope defined now  :)]
To further-clarify the goal here: right now, import-tests.py breaks some multicol tests when it imports them, so that they end up with XML parse errors from having "--" inside of an attribute in some cases, and bogus "-moz--moz-column-*" CSS in other cases.  In this bug, I intend to fix import-tests.py so that it doesn't introduce those problems.  Then after that's fixed, we can import the CSSWG multicol reftests (without introducing these problems!) in Bug 1295261
Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review76044

::: layout/reftests/w3c-css/import-tests.py:254
(Diff revision 2)
>              replacementLine = replacementLine.replace(rule, "-moz-" + rule)
> +            replacementLine = replacementLine.replace("--moz-", "-moz-")
> +            replacementLine = replacementLine.replace("-moz-moz-", "-moz-")

So instead of doing this, I think it would be better to change the first line so that the prefixed properties are only replaced when they're an entire word, rather than part of a word.  In particular, I think this would mean doing something more like just replacing the first line with:

replacementLine = re.sub("\\b" + rule + "\\b", "-moz-" + rule, replacementLine)

(I think it's also better to use something more like "for prop in aProps" rather than "for rule in aProps".)

If that works (which is partly judged by what changes in the imported tests it produces, if any), then r=dbaron with that.
Attachment #8787013 - Flags: review?(dbaron) → review+
Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review76044

> So instead of doing this, I think it would be better to change the first line so that the prefixed properties are only replaced when they're an entire word, rather than part of a word.  In particular, I think this would mean doing something more like just replacing the first line with:
>
> replacementLine = re.sub("\\b" + rule + "\\b", "-moz-" + rule, replacementLine)
>
>
> (I think it's also better to use something more like "for prop in aProps" rather than "for rule in aProps".)

So I tried using the re.sub with \b but I found some issues with it. Because of this I had to make the following additions:

1. Added "if( re.match(".*href=.*", replacementLine) is None):" because '\b' was also matching cases when the href had dashes eg: href="path\to\file\multicol-columns-test.html" To prevent this line from being changed, I basically skipped all lines with href which happen to be all the lines with this meta data (did a manual check)

2. The other two lines i.e. ->
replacementLine = replacementLine.replace("--moz-", "-moz-")
replacementLine = replacementLine.replace("-moz-moz-", "-moz-")
had to be kept for cases when the same line matches more than one rule, leading to prefixes like -moz--moz- eg: Rule "column-rule" is a subset of rules "column-rule-color", "column-rule-style" and "column-rule-width". So, something like this would happen:
column-rule-color -> -moz-column-rule-color -> -moz--moz-column-rule-color

With these changes above, the treeherder build looks pretty good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf8c13d09ab

Let me know what you think. :)
Made some more changes related to the same fix. Even though this was already 'r+'ed by you, thought it would be a good idea to run the changes by you again.
Comment #9 is no longer relevant.
Flags: needinfo?(dbaron)
Some additional context (from talking with Neerja):
- MozReview doesn't seem to support re-requesting review when r+ has already been granted; hence, I suggested that Neerja co-opt the "needinfo" flag to get this back in your review queue (effectively).
- The "\\b" that you suggested in comment 7 didn't end up helping, because python treats hyphens as a word-boundary.  (So all the false-positive matches were still matching.)  "[^-#]" seems to be sufficient to exclude all the places we want to exclude, though. (Excluding "-" lets us avoid matching href="multicol-columns-whatever.html" strings, and also avoid redundantly matching the same substring [which previously produced some "-moz-moz" issues].  And excluding "#" lets us avoid matching anchors in links to spec sections.)
Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review80110

This seems a little risky, since a CSS property could certainly come at the start of a line (e.g., in a style sheet that wasn't indented).

Could you add code that also does the substitution if it happens at the start of the line?

(Also, it would be good to see the diff to the imported tests at the same time as this fix.)

r=dbaron with that
Flags: needinfo?(dbaron)
Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review80110

I added the change to match start of line. Right now there is no test that matches this case but I tested this fix by editing a test locally and I see that it does get substituted correcty with this change.
Also, since there was no change in the imported tests, I didn't repush those patches.

But, all reimports of tests after applying any patch to this bug can be seen in the following patch:
Bug 1295261 - Reimport multicol tests after applying fix for import-tests.py (Bug 1299012)

*This patch is unchanged right now since no test matched the start of line case on rerunning import-tests.py
Looks good.  Thanks.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/409769321b11
Fix import-tests.py to avoid breaking three multicol reftests r=dbaron
https://hg.mozilla.org/mozilla-central/rev/409769321b11
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.