Update test262 and test262 skipped files

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: leonardo.balter, Unassigned)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 14 obsolete attachments)

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
(Reporter)

Description

a year ago
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
```
(Reporter)

Comment 1

a year ago
Created attachment 8864641 [details] [diff] [review]
0002-Update-skip-list.patch
(Reporter)

Comment 2

a year ago
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.
(Reporter)

Comment 3

a year ago
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
(Reporter)

Comment 4

a year ago
Created attachment 8864642 [details] [diff] [review]
0001-Update-test262-files.patch.zip
(Reporter)

Comment 5

a year ago
ok, it's zipped. I hope it works.
(Reporter)

Comment 6

a year ago
Created attachment 8864975 [details] [diff] [review]
0001-Update-test262-files.patch.zip
Attachment #8864642 - Attachment is obsolete: true
(Reporter)

Comment 7

a year ago
Created attachment 8864976 [details] [diff] [review]
0002-Update-skip-list.patch
Attachment #8864641 - Attachment is obsolete: true
(Reporter)

Comment 8

a year ago
I updated the files with the hg patches.

Comment 9

a year ago
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/
(Reporter)

Comment 10

a year ago
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
(Reporter)

Comment 11

a year ago
Created attachment 8865613 [details] [diff] [review]
patch.zip

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
(Reporter)

Updated

a year ago
Attachment #8865613 - Flags: review?(shu)
(Reporter)

Comment 12

a year ago
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).
(Reporter)

Comment 15

a year ago
> 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.
(Reporter)

Comment 17

a year ago
Created attachment 8866038 [details] [diff] [review]
0001-Bug-1362169-Update-test262-files-r-shu.patch.zip

First commit, update test262
Attachment #8865613 - Attachment is obsolete: true
Attachment #8865613 - Flags: review?(shu)
Attachment #8866038 - Flags: review?(shu)
(Reporter)

Comment 18

a year ago
Created attachment 8866039 [details] [diff] [review]
0002-Bug-1362169-Update-skip-list-r-shu.patch

Second commit, updates the skip list file.
Attachment #8866039 - Flags: review?(shu)
(Reporter)

Comment 19

a year ago
>  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.
(Reporter)

Comment 20

a year ago
Created attachment 8866043 [details] [diff] [review]
0002-Bug-1362169-Update-skip-list-r-shu.patch

updated skip list using folders
Attachment #8866039 - Attachment is obsolete: true
Attachment #8866039 - Flags: review?(shu)
Attachment #8866043 - Flags: review?(shu)
(Reporter)

Comment 21

a year ago
Created attachment 8866056 [details] [diff] [review]
0002-Bug-1362169-Update-skip-list-r-shu.patch

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 23

a year ago
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 24

a year ago
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)

Comment 25

a year ago
(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.

Updated

a year ago
Attachment #8866038 - Flags: review+
(Reporter)

Comment 26

a year ago
Created attachment 8866084 [details] [diff] [review]
0001-Bug-1362169-Update-Test262-files-r-shu.patch.zip

this one does not remove the test262/local folder
Attachment #8866038 - Attachment is obsolete: true
Attachment #8866084 - Flags: review?(shu)
(Reporter)

Comment 27

a year ago
Created attachment 8866085 [details] [diff] [review]
0002-Bug-1362169-Remove-test262-local-files-r-shu.patch

remove test262/local files that are already superseded
(Reporter)

Comment 28

a year ago
Created attachment 8866086 [details] [diff] [review]
0002-Bug-1362169-Remove-test262-local-files-r-shu.patch

this one removes the superseded files and has a reviewer assigned.
Attachment #8866085 - Attachment is obsolete: true
Attachment #8866086 - Flags: review?(shu)
(Reporter)

Comment 29

a year ago
Created attachment 8866087 [details] [diff] [review]
0003-Bug-1362169-Update-tests-skip-list-r-shu.patch
Attachment #8866056 - Attachment is obsolete: true
(Reporter)

Comment 30

a year ago
Created attachment 8866088 [details] [diff] [review]
0003-Bug-1362169-Update-tests-skip-list-r-shu.patch
Attachment #8866087 - Attachment is obsolete: true
Attachment #8866088 - Flags: review?(shu)

Comment 32

a year ago
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+

Updated

a year ago
Attachment #8866086 - Flags: review?(shu) → review+

Updated

a year ago
Attachment #8866088 - Flags: review?(shu) → review+

Updated

a year ago
Attachment #8866084 - Flags: review?(shu) → review+
(Reporter)

Comment 33

a year ago
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.
(Reporter)

Comment 35

a year ago
Created attachment 8866421 [details] [diff] [review]
0003-Bug-1362169-Update-tests-skip-list-r-shu.patch

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)
(Reporter)

Comment 36

a year ago
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
Attachment #8866421 - Attachment is obsolete: true
Attachment #8866421 - Flags: review?(shu)
Attachment #8866436 - Flags: review?(shu)
(Reporter)

Comment 37

a year ago
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.
(Reporter)

Comment 39

a year ago
Created attachment 8866943 [details] [diff] [review]
0003-Bug-1362169-Update-tests-skip-list-r-shu.patch

rollback the skip list for Simd tests.
Attachment #8866436 - Attachment is obsolete: true
Attachment #8866436 - Flags: review?(shu)
Attachment #8866943 - Flags: review?(shu)

Updated

a year ago
Attachment #8866943 - Flags: review?(shu) → review+

Comment 42

a year ago
bugherder
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
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 43

a year ago
thank you!
You need to log in before you can comment on or make changes to this bug.