Closed Bug 1331846 Opened 3 years ago Closed 3 years ago

Error when running layout/reftests/w3c-css/submitted/check-for-references.sh

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(1 file)

$ layout/reftests/w3c-css/submitted/check-for-references.sh 
Unexpected type skip-if(winWidget&&isDebugBuild) for ./include
grep: ./include: No such file or directory
Missing link for ./include
Unexpected type skip-if(winWidget&&isDebugBuild) for ./include
grep: ./include: No such file or directory
Missing link for ./include

That's because the "skip-if(winWidget&&isDebugBuild)" added before the "include", which causes the script fails.
http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/reftests/w3c-css/submitted/reftest.list#58,66
David, I need your help to review check-for-references.sh because you're the author.
Flags: needinfo?(dbaron)
Comment on attachment 8827740 [details]
Bug 1331846 - Exclude the lines which have "include" appeared in the middle.

https://reviewboard.mozilla.org/r/105350/#review106506

::: layout/reftests/w3c-css/submitted/check-for-references.sh:6
(Diff revision 1)
>  #!/bin/bash
>  
>  cd "$(dirname "$0")"
>  find . -name reftest.list | sed 's,/reftest.list$,,' | while read DIRNAME
>  do
> -    cat "$DIRNAME/reftest.list" | grep -v "^\(include\|default-preferences\)" | sed 's/ #.*//;s/^#.*//;s/.* == /== /;s/.* != /!= /' | grep -v "^ *$" | while read TYPE TEST REF
> +    cat "$DIRNAME/reftest.list" | grep -v "\(include\|default-preferences\)" | sed 's/ #.*//;s/^#.*//;s/.* == /== /;s/.* != /!= /' | grep -v "^ *$" | while read TYPE TEST REF

This seems a little too broad -- removing anything with "include" might remove some tests.

I'd prefer if you:
 - require a space after
 - require either a space or start-of-line before

Though if the latter is hard, I'd probably be OK with just the former.
Attachment #8827740 - Flags: review-
Flags: needinfo?(dbaron)
Comment on attachment 8827740 [details]
Bug 1331846 - Exclude the lines which have "include" appeared in the middle.

https://reviewboard.mozilla.org/r/105350/#review106506

> This seems a little too broad -- removing anything with "include" might remove some tests.
> 
> I'd prefer if you:
>  - require a space after
>  - require either a space or start-of-line before
> 
> Though if the latter is hard, I'd probably be OK with just the former.

I'll go with the former "require a space after". If the test is `== include.html include-ref.html`, it will be removed by the latter method because there's a space before the "include.html"

In patchset 2, I split the "include" and "default-preference" matching rules because a standalone "default-preferences" [1] is allowed.

[1] https://dxr.mozilla.org/mozilla-central/rev/b3774461acc6bee2216c5f57e167f9e5795fb09d/layout/reftests/w3c-css/submitted/masking/reftest.list#108
Addressed the comments in comment 3. Please take a look again.
Flags: needinfo?(dbaron)
Comment on attachment 8827740 [details]
Bug 1331846 - Exclude the lines which have "include" appeared in the middle.

https://reviewboard.mozilla.org/r/105350/#review106734

Ship It!
Attachment #8827740 - Flags: review-
Comment on attachment 8827740 [details]
Bug 1331846 - Exclude the lines which have "include" appeared in the middle.

https://reviewboard.mozilla.org/r/105350/#review106738
Attachment #8827740 - Flags: review- → review+
Flags: needinfo?(dbaron)
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe22af79bacf
Exclude the lines which have "include" appeared in the middle. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/fe22af79bacf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.