Closed Bug 1430939 Opened 6 years ago Closed 6 years ago

Import-tests.py broken due to wrong directory names after some directories were renamed in github repo w3c/web-platform-tests. Fix this and update existing tests using import-tests.py

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: neerja, Assigned: neerja)

References

Details

Attachments

(6 files)

Bug 1430939 - Part1:Update directory names in import-tests.py and failures.list to match names in w3c/web-platform-tests.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1430939 - Part2:Rename directories in reftests/w3c/received/ to drop version number.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1430939 - Part3:Pull latest tests from w3c/web-platform-tests using import-tests.py and update failures.list.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1430939 - Part4:Transfer styloVsGecko annotation from directly in reftest.list to failures.list so that it cannot be accidently overwritten when running import-tests.py.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1430939 - Part5:Remove or update annotations of tests in failures.list that pass after being updated from w3c/web-platform-tests.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1430939 - Part6:Change reftests/moz.build to use the new directory names without version number.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
      No description provided.
Summary: Import-tests.py broken due to wrong directory names since github repo w3c/csswg-test was merged into w3c/web-platform-tests. → Import-tests.py broken due to wrong directory names since github repo w3c/csswg-test was merged into w3c/web-platform-tests. Fix this and update existing tests using import-tests.py
Comment on attachment 8943083 [details]
Bug 1430939 - Part1:Update directory names in import-tests.py and failures.list to match names in w3c/web-platform-tests.

https://reviewboard.mozilla.org/r/213380/#review219366
Attachment #8943083 - Flags: review?(dholbert) → review+
> → Import-tests.py broken due to wrong directory names since github repo
> w3c/csswg-test was merged into w3c/web-platform-tests.

BTW, it looks like this broke more recently than that merge.

 (1) We switched to pull from w3c/web-platform-tests (rather than w3c/csswg-test) back in bug 1362255, and everything seems to have been working fine at that point.

 (2) Later on (in October), these directories were all renamed (in web-platform-tests) to drop the verison number, here: https://github.com/w3c/web-platform-tests/issues/7503

So change (2) is the specific thing that we're cleaning up after here, it looks like.
See Also: → https://github.com/w3c/web-platform-tests/issues/7503
Comment on attachment 8943095 [details]
Bug 1430939 - Part2:Rename directories in reftests/w3c/received/ to drop version number.

https://reviewboard.mozilla.org/r/213390/#review219388

Based on the commit message, I initially assumed this patch was entirely (or almost-entirely) automated, and was generated by simply running "import-tests.py".

But it looks like that's not the case -- when I run "import-tests.py" locally (in a tree that has your "Part 1" patch), I get every single existing test being deleted (from their version-numbered-directories) and then recreated in the new unversioned directories.  So I'm guessing you also had to run some manual "hg mv" (aka "hg rename") commands, to update all the directories and to get this represented as file-moves.

It's good to have them represented as file-moves, but we shouldn't mix manual & automated steps together -- for a patch this large (mozreview says "This diff has been split across 24 pages"), it'd be better to make an *extremely* clean break between automated/script-generated parts vs. manually generated parts (even with manual "hg rename" commands).  So -- could you split this part up as follows:
 Part 2: Rename directories in reftests/w3c/received/ to drop version number
 Part 3: Run import-tests.py to pull in updated tests from w3c/web-platform/tests

(Unless I'm mistaken and there's some way of running import-tests.py that does in fact generate this whole patch as a one-off...)
Attachment #8943095 - Flags: review?(dholbert) → review-
>  Part 3: Run import-tests.py to pull in updated tests from w3c/web-platform/tests

(sorry -- s/pull in updated tests/update existing tests/ -- it looks like you're wanting to hold off on importing new tests, which seems fine for the purposes of this bug)
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 8943095 [details]
> Bug 1430939 - Part2:Update all existing tests with the latest in
> w3c/web-platform-tests using import-tests.py (without adding any new tests).
> 
> https://reviewboard.mozilla.org/r/213390/#review219388

Yeah, I did have to use hg rename for the six directories inside the received folder. I thought this would be ok since most of the test files are unchanged but show up in the MozReview diff just because of the directory rename, hence the 24 pages. But no big deal, I can separate this out. :)
Cool! Thanks. The reason I'm requesting that is: I can't usefully review a 24-page mozreview request. :)  But I *can* review a few automated commands as being sane, and then I can validate that the patch is correct by running those commands locally and diffing the result against the patch that you posted.

That lets me make an educated "r+" judgement on patches such as these, without having to physically read through the whole patch.
Summary: Import-tests.py broken due to wrong directory names since github repo w3c/csswg-test was merged into w3c/web-platform-tests. Fix this and update existing tests using import-tests.py → Import-tests.py broken due to wrong directory names after some directories were renamed in github repo w3c/web-platform-tests. Fix this and update existing tests using import-tests.py
(In reply to Daniel Holbert [:dholbert] from comment #9)
> Cool! Thanks. The reason I'm requesting that is: I can't usefully review a
> 24-page mozreview request. :)  But I *can* review a few automated commands
> as being sane, and then I can validate that the patch is correct by running
> those commands locally and diffing the result against the patch that you
> posted.
> 
> That lets me make an educated "r+" judgement on patches such as these,
> without having to physically read through the whole patch.

Yes, now that I restructured the patch, I understand that is does make more sense than the one I had before. :) Thanks!
Comment on attachment 8943452 [details]
Bug 1430939 - Part4:Transfer styloVsGecko annotation from directly in reftest.list to failures.list so that it cannot be accidently overwritten when running import-tests.py.

https://reviewboard.mozilla.org/r/213788/#review219558

Looks good, thanks!
Comment on attachment 8943452 [details]
Bug 1430939 - Part4:Transfer styloVsGecko annotation from directly in reftest.list to failures.list so that it cannot be accidently overwritten when running import-tests.py.

https://reviewboard.mozilla.org/r/213788/#review219560
Attachment #8943452 - Flags: review?(emilio) → review+
Comment on attachment 8943095 [details]
Bug 1430939 - Part2:Rename directories in reftests/w3c/received/ to drop version number.

https://reviewboard.mozilla.org/r/213390/#review219572
Attachment #8943095 - Flags: review?(dholbert) → review+
Just a heads-up -- I think "part 1" (at least) needs a rebase before it can be autolanded, BTW. (I tried applying this patch stack on top of current mozilla-central, but that first part failed to apply -- looks like it had some recent changes in other bugs, per https://hg.mozilla.org/mozilla-central/filelog/tip/layout/reftests/w3c-css/failures.list , which probably caused the conflicts.)
Flags: needinfo?(npancholi)
Comment on attachment 8943451 [details]
Bug 1430939 - Part3:Pull latest tests from w3c/web-platform-tests using import-tests.py and update failures.list.

https://reviewboard.mozilla.org/r/213786/#review219580

::: layout/reftests/w3c-css/failures.list:320
(Diff revision 1)
> +skip css-writing-modes/reference/bidi-table-001.html
> +skip css-writing-modes/reference/block-plaintext-006.html

These two files (in the "reference" subdir) seem to be reference cases, not testcases -- so you don't need to list them here.

::: layout/reftests/w3c-css/failures.list:324
(Diff revision 1)
> +skip css-writing-modes/block-plaintext-006.html
> +skip css-writing-modes/reference/bidi-table-001.html
> +skip css-writing-modes/reference/block-plaintext-006.html
> +skip css-writing-modes/table-cell-001.html
> +skip css-writing-modes/table-cell-002.html
> +skip reference/nothing.html

Similarly, this file (nothing.html) is only used as a reference case AFAICT, so you shouldn't need to list it here.

::: layout/reftests/w3c-css/failures.list:332
(Diff revision 1)
> +skip selectors/selector-read-write-type-change-002.html
> +skip selectors/selector-required-type-change-001.html
> +skip selectors/selector-required-type-change-002.html
> +skip selectors/selectors-attr-white-space-001.html
> +skip selectors/selectors-empty-001.xml
> +skip selectors/selectors-namespace-001.xml

Please add a newline character at the end of this file, as noted at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices

(MozReview doesn't show it, but right now this patch is leaving this failures.list file without a newline at the end, which makes raw-text patches include the cruft "\ No newline at end of file", as you can see at https://reviewboard-hg.mozilla.org/gecko/raw-rev/90a4b662beaab2b93659f6708b1b6b224f520983 if you scroll down a little bit.)

(A lot of the imported test files themselves have the same issue, but that's not your fault & not something that we'd want to fix locally. But failures.list is our own file & we should keep it good. :))

::: layout/reftests/w3c-css/received/import.log:1
(Diff revision 1)
> -Importing revision: a574530634a7e3f1c4e4f0385ec8e32620468def
> -from repository: https://github.com/w3c/web-platform-tests.git
> +Importing revision: 15f199c91a72b0d51bf0a12b3b77827ecb5051ff
> +from repository: https://github.com/neerjapancholi/web-platform-tests.git

Based on this line in the log, it looks like you ran this script on a clone of your own fork, rather than the official w3c repo.

This probably isn't a huge deal, particularly since it looks like the imported revision is a commit that did make it upstream.  So this doesn't cause any problems other than having this slightly unexpected line in the log, and it's not worth asking you to regenerate the patch (including new files that have arrived upstream, and it looks like there are a few) with the correct repo.  So, I don't think you need to worry about this for now [hence I'm not ticking the "open an issue" mozreview checkbox]

But for future imports, please do use a clone of the canonical github repo. :)
Attachment #8943451 - Flags: review?(dholbert) → review+
Comment on attachment 8943732 [details]
Bug 1430939 - Part5:Remove or update annotations of tests in failures.list that pass after being updated from w3c/web-platform-tests.

https://reviewboard.mozilla.org/r/214118/#review219838
Attachment #8943732 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #18)
> Just a heads-up -- I think "part 1" (at least) needs a rebase before it can
> be autolanded, BTW. (I tried applying this patch stack on top of current
> mozilla-central, but that first part failed to apply -- looks like it had
> some recent changes in other bugs, per
> https://hg.mozilla.org/mozilla-central/filelog/tip/layout/reftests/w3c-css/
> failures.list , which probably caused the conflicts.)

Yeah, it did need to rebase it, thanks for catching that before I pushed this! :)
I also added Part5 and Part6 to fix these failures on try -
https://treeherder.mozilla.org/#/jobs?repo=try&revision=612bf41bfac0dbf2ef741ec0c9e3ff32fce5b18a&selectedJob=157033308
Flags: needinfo?(npancholi)
Comment on attachment 8943734 [details]
Bug 1430939 - Part6:Change reftests/moz.build to use the new directory names without version number.

https://reviewboard.mozilla.org/r/214120/#review219840

Good catch! r=me
Attachment #8943734 - Flags: review?(dholbert) → review+
Pushed by npancholi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86576ceab319
Part1:Update directory names in import-tests.py and failures.list to match names in w3c/web-platform-tests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6c200b73f15d
Part2:Rename directories in reftests/w3c/received/ to drop version number. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a5f53a9fb044
Part3:Pull latest tests from w3c/web-platform-tests using import-tests.py and update failures.list. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ba9ef655e40b
Part4:Transfer styloVsGecko annotation from directly in reftest.list to failures.list so that it cannot be accidently overwritten when running import-tests.py. r=emilio
https://hg.mozilla.org/integration/autoland/rev/75e2e449d517
Part5:Remove or update annotations of tests in failures.list that pass after being updated from w3c/web-platform-tests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c203cfd5716b
Part6:Change reftests/moz.build to use the new directory names without version number. r=dholbert
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/55e8bfe2dc42
Fix reftest failures discovered after rechunking. r=kats a=test-fix on a CLOSED TREE
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/8cde429207f3
Fix failure in reftest select-disabled-fieldset-1.html discovered after rechunking. r=me a=test-fix on a CLOSED TREE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: