Closed Bug 1423335 Opened 2 years ago Closed 2 years ago

Reorganization of js/src/tests/...

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: valerie, Assigned: valerie)

Details

Attachments

(17 files, 14 obsolete files)

23.02 KB, application/x-shellscript
Details
641.42 KB, patch
sfink
: review+
Details | Diff | Splinter Review
9.42 KB, patch
sfink
: review+
Details | Diff | Splinter Review
22.18 KB, patch
sfink
: review+
Details | Diff | Splinter Review
13.17 KB, patch
sfink
: review+
Details | Diff | Splinter Review
20.41 KB, patch
sfink
: review+
Details | Diff | Splinter Review
10.22 KB, patch
sfink
: review+
Details | Diff | Splinter Review
29.71 KB, patch
sfink
: review+
Details | Diff | Splinter Review
8.85 KB, patch
sfink
: review+
Details | Diff | Splinter Review
58.67 KB, patch
sfink
: review+
Details | Diff | Splinter Review
40.78 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.59 KB, patch
sfink
: review+
Details | Diff | Splinter Review
4.20 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.54 MB, patch
sfink
: review+
Details | Diff | Splinter Review
1.92 KB, patch
sfink
: review+
Details | Diff | Splinter Review
5.41 KB, patch
sfink
: review+
Details | Diff | Splinter Review
23.71 KB, patch
sfink
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8934730 - Flags: review?(sphink)
Attachment #8934732 - Flags: review?(sphink)
Attachment #8934733 - Flags: review?(sphink)
Attachment #8934734 - Flags: review?(sphink)
Attachment #8934735 - Flags: review?(sphink)
Attachment #8934736 - Flags: review?(sphink)
Attachment #8934737 - Flags: review?(sphink)
Attachment #8934738 - Flags: review?(sphink)
Attachment #8934740 - Flags: review?(sphink)
Attachment #8934726 - Flags: review?(sphink)
Attachment #8934727 - Flags: review?(sphink)
Attachment #8934728 - Flags: review?(sphink)
Attachment #8934729 - Flags: review?(sphink)
The original plan for this re-org -- specifically, which folders to move tests to and which folders to delete -- can be found here:
https://docs.google.com/a/bocoup.com/spreadsheets/d/1vLau5D2iqS3PAnh9ptr29QDGZBOYl6v42UXYR3qyQ5k/edit?usp=sharing

See the attachment for the steps I took to do the re-org, which might help with the review.

Sorry there are so many patches! I want to keep the shell.js file recombination grouped in a way to make the history easy to follow.
Comment on attachment 8934726 [details] [diff] [review]
0001-Bug-1423335-jstest-reorg-move-all-test-files-r-sfink.patch

Waldo - asking for your feedback requesteing feedback for your opinino on the directory name for non-test262 tests. Currently, they're in ecma* or js*. The current patch moves them all under sm/. Other possibilities -- spidermonkey/, custom/, regress/, topic/, area/, test/, local/, non262/.

They aren't all regression tests, though I suppose we could think of them that way.
Attachment #8934726 - Flags: feedback?(jwalden+bmo)
Assignee: nobody → valerie
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Some of the patches cannot be viewed in bugzilla ("No valid patch files were found in the attachment.").

And we probably want to keep some of the files deleted in part 11.
- There are some newer files like:
 - js/src/tests/ecma/Array/array-length-set-during-for-in.js
 - js/src/tests/ecma/Array/array-length-set-on-nonarray.js
 - Basically all files which don't follow the <section>-<step>.js nor the <name>-<counter>.js syntax?
- Tests for the (currently) non-standard RegExp statics in js/src/tests/js1_2/regexp/RegExp_*.js
(In reply to Steve Fink [:sfink] [:s:] from comment #16)
> Waldo - asking for your feedback requesteing feedback for your opinino on
> the directory name for non-test262 tests. Currently, they're in ecma* or
> js*. The current patch moves them all under sm/. Other possibilities --
> spidermonkey/, custom/, regress/, topic/, area/, test/, local/, non262/.

Everything in our tree is SM, so sm/ seems a little weird.  non262/ seems fine by me, others would work too but not worth the time to ponder.
(In reply to André Bargull [:anba] from comment #17)
> Some of the patches cannot be viewed in bugzilla ("No valid patch files were
> found in the attachment.").

I never saw that using the Splinter Review link, though some of them came up empty because it seems Splinter doesn't do anything with renames.

You can always look at the raw attachment; it's easier to read for many of these anyway.
(In reply to André Bargull [:anba] from comment #17)
> Some of the patches cannot be viewed in bugzilla ("No valid patch files were
> found in the attachment.").
> 
> And we probably want to keep some of the files deleted in part 11.
> - There are some newer files like:
>  - js/src/tests/ecma/Array/array-length-set-during-for-in.js
>  - js/src/tests/ecma/Array/array-length-set-on-nonarray.js
>  - Basically all files which don't follow the <section>-<step>.js nor the
> <name>-<counter>.js syntax?
> - Tests for the (currently) non-standard RegExp statics in
> js/src/tests/js1_2/regexp/RegExp_*.js

Hey Anba, I really appreciate this feedback!

The tests I'm now saving, following your advice, are the following:

ecma/Array/array-length-set-during-for-in.js
ecma/Array/array-length-set-on-nonarray.js
ecma/extensions/errorcolumnblame.js
ecma/Number/0x-without-following-hexdigits.js
ecma/String/lastIndexOf-ToNumber-when-searchStr-larger-than-string.js
js1_2/regexp/RegExp_dollar_number.js
js1_2/regexp/RegExp_lastMatch_as_array.js
js1_2/regexp/RegExp_lastMatch.js
js1_2/regexp/RegExp_lastParen_as_array.js
js1_2/regexp/RegExp_lastParen.js
js1_2/regexp/RegExp_leftContext_as_array.js
js1_2/regexp/RegExp_leftContext.js
js1_2/regexp/RegExp_object.js
js1_2/regexp/RegExp_rightContext_as_array.js
js1_2/regexp/RegExp_rightContext.js

A handful of other recent tests added to those directories I had already pulled out for saving. The only files now being deleted follow the naming format you called out (<section>-<step>.js or <name>-<counter>.js) OR reference bugs in Netscape (not findable in b.m.o).

Aside: I saved tests from those directories that test implementation-defined areas of ECMAScript under Array, Math, Date. You can see these listed in the "Save implementation-defined tests" in the text file attachment, if you are curious.
Attachment #8934726 - Attachment is obsolete: true
Attachment #8934726 - Flags: review?(sphink)
Attachment #8934726 - Flags: feedback?(jwalden+bmo)
Attachment #8935505 - Flags: review?(sphink)
Attachment #8934727 - Attachment is obsolete: true
Attachment #8934727 - Flags: review?(sphink)
Attachment #8935507 - Flags: review?(sphink)
Attachment #8934728 - Attachment is obsolete: true
Attachment #8934728 - Flags: review?(sphink)
Attachment #8935508 - Flags: review?(sphink)
Attachment #8934729 - Attachment is obsolete: true
Attachment #8934729 - Flags: review?(sphink)
Attachment #8935510 - Flags: review?(sphink)
Attachment #8934730 - Attachment is obsolete: true
Attachment #8934730 - Flags: review?(sphink)
Attachment #8935511 - Flags: review?(sphink)
Attachment #8934732 - Attachment is obsolete: true
Attachment #8934732 - Flags: review?(sphink)
Attachment #8935512 - Flags: review?(sphink)
Attachment #8934733 - Attachment is obsolete: true
Attachment #8934733 - Flags: review?(sphink)
Attachment #8935514 - Flags: review?(sphink)
Attachment #8934734 - Attachment is obsolete: true
Attachment #8934734 - Flags: review?(sphink)
Attachment #8935515 - Flags: review?(sphink)
Attachment #8934735 - Attachment is obsolete: true
Attachment #8934735 - Flags: review?(sphink)
Attachment #8935517 - Flags: review?(sphink)
Attachment #8934736 - Attachment is obsolete: true
Attachment #8934736 - Flags: review?(sphink)
Attachment #8934738 - Attachment is obsolete: true
Attachment #8934738 - Flags: review?(sphink)
Attachment #8935519 - Flags: review?(sphink)
Attachment #8934740 - Attachment is obsolete: true
Attachment #8934740 - Flags: review?(sphink)
Attachment #8935521 - Flags: review?(sphink)
Attachment #8934737 - Attachment is obsolete: true
Attachment #8934737 - Flags: review?(sphink)
Attachment #8935524 - Flags: review?(sphink)
There are a few references to the files I move in the jit-test directories
Attachment #8935529 - Flags: review?(sphink)
Hi Steve -- here are all the commits, re-uploaded with the path change.

To change the directory -- rather than make a new patch, or go back and re-run all the git commands AND edit some files by hand -- I tried to simply replace the references to "sm/" to "non262/" in the patch text files. If this doesn't work for some reason, we can go with the original patch set and I will add a commit changing the directories.

Excited to see this land!
Attachment #8935505 - Flags: review?(sphink) → review+
Attachment #8935507 - Flags: review?(sphink) → review+
Attachment #8935508 - Flags: review?(sphink) → review+
Attachment #8935510 - Flags: review?(sphink) → review+
Comment on attachment 8935511 [details] [diff] [review]
0005-Bug-1423335-jstest-reorg-move-funcs-in-js1_5-Express.patch

Review of attachment 8935511 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/non262/expressions/shell.js
@@ +183,5 @@
> +
> +(function(global) {
> +  /* This Source Code Form is subject to the terms of the Mozilla Public
> +   * License, v. 2.0. If a copy of the MPL was not distributed with this
> +   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This license header shouldn't be here; there should just be one at the top of the file.
Attachment #8935511 - Flags: review?(sphink) → review+
Attachment #8935512 - Flags: review?(sphink) → review+
Attachment #8935514 - Flags: review?(sphink) → review+
Attachment #8935515 - Flags: review?(sphink) → review+
Attachment #8935517 - Flags: review?(sphink) → review+
Comment on attachment 8935518 [details] [diff] [review]
0010-Bug-1423335-jstest-reorg-remove-unnecessary-files-r-.patch

Note to self: look closer to see if we're losing something with js/src/tests/ecma_7/TypedArray/shell.js
Move the copyright to the top of the file in non262/expression/shell.js
Attachment #8935511 - Attachment is obsolete: true
Attachment #8935826 - Flags: review?(sphink)
Move Intl directory into non262/Intl -- as there is an analogous directory in test262 for Intl tests.
Comment on attachment 8935519 [details] [diff] [review]
0011-Bug-1423335-jstest-reorg-update-script-locations-fro.patch

Review of attachment 8935519 [details] [diff] [review]:
-----------------------------------------------------------------

Though many of these could be made unnecessary by changing load() to loadRelativeToScript(). I'll do that before landing.
Attachment #8935519 - Flags: review?(sphink) → review+
Attachment #8935521 - Flags: review?(sphink) → review+
Attachment #8935522 - Flags: review?(sphink) → review+
Comment on attachment 8935524 [details] [diff] [review]
0014-Bug-1423335-jstest-reorg-delete-old-test-files-cover.patch

I can't say I looked at this exhaustively, but I did a bunch of spot checking and scanning over names of things, and everything I saw seemed reasonable.
Attachment #8935524 - Flags: review?(sphink) → review+
Comment on attachment 8935529 [details] [diff] [review]
0015-Bug-1423335-jstest-reorg-fix-references-to-scripts-f.patch

Review of attachment 8935529 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/lib/match.js
@@ +1,1 @@
> +loadRelativeToScript("../../tests/non262/reflect-parse/Match.js");

Ugh. We probably ought to have something nicer for this, but this is totally fine for this bug.
Attachment #8935529 - Flags: review?(sphink) → review+
Attachment #8935826 - Flags: review?(sphink) → review+
Attachment #8935828 - Flags: review+
Attachment #8935518 - Flags: review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa2f59dee74
jstest reorganization: move all test files into non262/ with a parallel structure to test262, removing tests that duplicate test262 coverage. r=sfink
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69fc75e98c9b
jstest reorganization: move all test files into non262/ with a parallel structure to test262, removing tests that duplicate test262 coverage. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1574b94caa84
Backed out changeset 0fa2f59dee74 for breaking stage-package.
https://hg.mozilla.org/mozilla-central/rev/69fc75e98c9b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
This has not landed yet. There are problems with the packaging and continuous integration. I still need to get the split-out JIT jobs working again; they seem to be using a test package that is missing non262/shell.js.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P3
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcf40cc57f8
jstest reorganization: move all test files into non262/ with a parallel structure to test262, removing tests that duplicate test262 coverage. r=sfink
https://hg.mozilla.org/mozilla-central/rev/ebcf40cc57f8
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.