Closed Bug 1362169 Opened 3 years ago Closed 3 years ago

Update test262 and test262 skipped files

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: leonardo.balter, Assigned: leonardo.balter)

Details

Attachments

(3 files, 14 obsolete files)

717.44 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.62 KB, patch
shu
: review+
Details | Diff | Splinter Review
24.59 KB, patch
shu
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36

Steps to reproduce:

- Updated Test262 through the current script file.
- Updated the skip list to reflect solved issues and new test files not addressed yet.


Actual results:

```
 ./jstests.py `which js` # build from today 2017-05-04
[30028|    0|    0| 1458] 100% ======================================>| 623.3s
PASS
```
Attached patch 0002-Update-skip-list.patch (obsolete) — Splinter Review
I cannot attach the first file, it's too big.

> The file you are trying to attach is 11478 kilobytes (KB) in size. Attachments cannot be more than 10240 KB. 
> We recommend that you store your attachment elsewhere and then paste the URL to this file on the attachment creation page in the appropriate text field. 
> Alternately, if your attachment is an image, you could convert it to a compressible format like JPG or PNG and try again.
I've got the files in a dropbox folder. I'll check out how to do a smaller patch in the meanwhile.

https://www.dropbox.com/sh/jcsu4ck8c7i6adl/AAC1EsQtthM8rTHbJgcb9RLda?dl=0
ok, it's zipped. I hope it works.
Attachment #8864642 - Attachment is obsolete: true
Attached patch 0002-Update-skip-list.patch (obsolete) — Splinter Review
Attachment #8864641 - Attachment is obsolete: true
I updated the files with the hg patches.
Leo, are these ready for review? The usual etiquette is to flag a designated reviewer (me in this case).

I also recommend pushing to try to make sure that the in-browser run also passes. You should be able to push to try yourself with your commit access.

Instructions are available at [1]. The trychooser [2] syntax I recommend is "try: -b do -p linux -u jsreftest -t none". That is, run jsreftests for both debug and optimized builds on linux. Linux and linux64 tends to have tends to have the shortest wait times. After you fire off a try build, please paste the link as a comment in this bug.

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try
[2] https://mozilla-releng.net/trychooser/
I've been trying to set everything up this morning and I've got some issues with the login. Now it looks like I have issues pushing from git. I'll try to apply the changes from HG and see how it goes.


https://gist.github.com/leobalter/8cdddf6040157c3183828d6d117c9b31
Attached patch patch.zip (obsolete) — Splinter Review
Update to date patch file from rebased branch. The zip contains two patch files inside.
Attachment #8864975 - Attachment is obsolete: true
Attachment #8864976 - Attachment is obsolete: true
Attachment #8865613 - Flags: review?(shu)
Where I got the patch and try server configuration all set, I need to check the errors from the tryserver results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48f72923d01ef652dc8a096737afc1876ef52d71&selectedJob=97420685

I'll ping back when it's solved.
(In reply to Leo Balter from comment #12)
> Where I got the patch and try server configuration all set, I need to check
> the errors from the tryserver results:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=48f72923d01ef652dc8a096737afc1876ef52d71&selectedJob=9
> 7420685
> 
> I'll ping back when it's solved.

There's no proper error message because I used the wrong jstests harness function in http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/js/src/tests/test262-host.js#192. I assumed a function called "reportFailure" would report a failure, but while working on bug 1346234 I realised it actually doesn't report a failure... :-/

Ignoring this test integration issue for now, the tests are failing in browser environments, because the window object has a "length" property, so the assertion in https://github.com/tc39/test262/blob/1fe991a00dde60c91111f18da95c71b7e208f264/test/language/expressions/async-generator/dstr-ary-ptrn-rest-obj-prop-id.js#L44 fails. This means the tests need to be added to this block http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/js/src/tests/jstests.list#88 in jstests.list.
Can you upload two separate patch files, so it's possible to use Splinter to review the skip list update?

- Please remove the exclusions for "Object Rest/Spread Properties", bug 1339395 has landed, so we can and should use these tests.
- It's possible to exclude whole directories, so you don't need to list every test file, e.g. "skip include test262/built-ins/RegExp/property-escapes/jstests.list" to skip all files in "property-escapes" and its sub-directories.


And the final commit message must contain the bug number and the reviewer (https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment).
> Can you upload two separate patch files, so it's possible to use Splinter to review the skip list update?

Does Splinter works even with zipped files? They are zipped as the test262 update patch has more than 10MB.

> It's possible to exclude whole directories, so you don't need to list every test file, e.g. "skip include test262/built-ins/RegExp/property-escapes/jstests.list" to skip all files in "property-escapes" and its sub-directories.

nice! thanks for letting me know.

> And the final commit message must contain the bug number and the reviewer

Sure!
(In reply to Leo Balter from comment #15)
> > Can you upload two separate patch files, so it's possible to use Splinter to review the skip list update?
> 
> Does Splinter works even with zipped files? They are zipped as the test262
> update patch has more than 10MB.

No, Splinter doesn't grok zip-files. What I meant is to upload two files, a zipped patch with the test262 update and normal patch file for the skip list update.
First commit, update test262
Attachment #8865613 - Attachment is obsolete: true
Attachment #8865613 - Flags: review?(shu)
Attachment #8866038 - Flags: review?(shu)
Second commit, updates the skip list file.
Attachment #8866039 - Flags: review?(shu)
>  What I meant is to upload two files, a zipped patch with the test262 update and normal patch file for the skip list update.

I imagined it could be that so we have two files now.
updated skip list using folders
Attachment #8866039 - Attachment is obsolete: true
Attachment #8866039 - Flags: review?(shu)
Attachment #8866043 - Flags: review?(shu)
I'm updating this file to use `skip include` instead of `skip script`.

One thing I notice is that I was able to just use `skip script` with the folder name, without adding jstests.list as the suffix. Anyway, I'm following the recommendation.
Attachment #8866043 - Attachment is obsolete: true
Attachment #8866043 - Flags: review?(shu)
Attachment #8866056 - Flags: review?(shu)
Comment on attachment 8866056 [details] [diff] [review]
0002-Bug-1362169-Update-skip-list-r-shu.patch

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

Looks good.
Attachment #8866056 - Flags: review?(shu) → review+
Comment on attachment 8866038 [details] [diff] [review]
0001-Bug-1362169-Update-test262-files-r-shu.patch.zip

I see removals and no additions (e.g., the renaming of the exsting tests to existing). Forgot to git add?
Attachment #8866038 - Flags: review?(shu)
(In reply to Shu-yu Guo [:shu] from comment #24)
> Comment on attachment 8866038 [details] [diff] [review]
> 0001-Bug-1362169-Update-test262-files-r-shu.patch.zip
> 
> I see removals and no additions (e.g., the renaming of the exsting tests to
> existing). Forgot to git add?

Oh no, nevermind, *I* used the wrong git status flags.
Attachment #8866038 - Flags: review+
this one does not remove the test262/local folder
Attachment #8866038 - Attachment is obsolete: true
Attachment #8866084 - Flags: review?(shu)
remove test262/local files that are already superseded
this one removes the superseded files and has a reviewer assigned.
Attachment #8866085 - Attachment is obsolete: true
Attachment #8866086 - Flags: review?(shu)
Attachment #8866056 - Attachment is obsolete: true
Attachment #8866087 - Attachment is obsolete: true
Attachment #8866088 - Flags: review?(shu)
Comment on attachment 8866085 [details] [diff] [review]
0002-Bug-1362169-Remove-test262-local-files-r-shu.patch

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

Did you check if the local versions differed from the upstreamed versions?
Attachment #8866085 - Flags: review+
Attachment #8866086 - Flags: review?(shu) → review+
Attachment #8866088 - Flags: review?(shu) → review+
Attachment #8866084 - Flags: review?(shu) → review+
that's confusing. I don't know what is happening at the results but at least a caught a bug on test262, where I was missing a $DONE call to promise failures in async tests. I'll get that fixed and updated.
(In reply to Leo Balter from comment #33)
> that's confusing. I don't know what is happening at the results ...

See comment #13: The failing tests expect a ReferenceError is thrown when resolving an identifier named "length". But in browser environments, this "length" identifier lookup will successfully resolve the "length" property of the window object, so no ReferenceError is thrown.
This skip list includes the cases where the `length` binding is using in destructuring tests.
Attachment #8866088 - Attachment is obsolete: true
Attachment #8866421 - Flags: review?(shu)
More updates after running the tests from the skip list.

The respective try run (still in progress): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e016277f711592c2eecd222b8d69732e25aed3c7
Attachment #8866421 - Attachment is obsolete: true
Attachment #8866421 - Flags: review?(shu)
Attachment #8866436 - Flags: review?(shu)
ok, the current patch is passing on the try server. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e016277f711592c2eecd222b8d69732e25aed3c7
(In reply to Leo Balter from comment #36)
> Created attachment 8866436 [details] [diff] [review]
> 0003-Bug-1362169-Update-tests-skip-list-r-shu.patch
> 
> More updates after running the tests from the skip list.
> 
> The respective try run (still in progress):
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e016277f711592c2eecd222b8d69732e25aed3c7

I'd probably keep the SIMD tests excluded, because the test cases in bug 1336991 are still failing, which means enabling the tests in test262 could lead to intermittent failures.
rollback the skip list for Simd tests.
Attachment #8866436 - Attachment is obsolete: true
Attachment #8866436 - Flags: review?(shu)
Attachment #8866943 - Flags: review?(shu)
Attachment #8866943 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/e3a43690488d
https://hg.mozilla.org/mozilla-central/rev/281ecf52a434
https://hg.mozilla.org/mozilla-central/rev/18198e29a2d5
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
thank you!
Assignee: nobody → leonardo.balter
You need to log in before you can comment on or make changes to this bug.