Closed Bug 1386026 Opened 2 years ago Closed 2 years ago

Various SM async tests are going to permafail when Gecko 56 merges to Beta

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 ? fixed

People

(Reporter: RyanVM, Assigned: jimb)

References

Details

Attachments

(3 files, 3 obsolete files)

[Tracking Requested - why for this release]: Permafailing SM test suites on August 2 when Gecko 56 is merged to Beta.

Looks like fallout from bug 1382258 and bug 1380498.

https://treeherder.mozilla.org/logviewer.html#?job_id=119709602&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=119709384&repo=try
Flags: needinfo?(jimb)
Also the jsreftest failure from below, which is from bug 1379717 I believe:
https://treeherder.mozilla.org/logviewer.html#?job_id=119721267&repo=try
Blocks: 1379717
Flags: needinfo?(andrebargull)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> Also the jsreftest failure from below, which is from bug 1379717 I believe:
> https://treeherder.mozilla.org/logviewer.html#?job_id=119721267&repo=try

Argh, yes it is. https://hg.mozilla.org/mozilla-central/annotate/3bceabbf445d/js/src/tests/ecma_6/extensions/newer-type-functions-caller-arguments.js#l66 actually has a function to feature test async generators, but fails to invoke it... :-/
Flags: needinfo?(andrebargull)
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Flags: needinfo?(jimb)
Attachment #8892296 - Flags: review?(ttromey)
Attachment #8892296 - Attachment is obsolete: true
Attachment #8892296 - Flags: review?(ttromey)
Attachment #8892298 - Flags: review?(ttromey)
Sorry, I wanted the patches to appear in the order they should be applied. (Maybe it's time to learn ReviewBoard.)

The first patch should have no effect on how the tests run. The second patch is the item of interest.
This is the patch for the issue mentioned in comment #1. 

The patch is trivial enough it shouldn't require any review.
Attachment #8892400 - Flags: review+
Comment on attachment 8892298 [details] [diff] [review]
Put Debugger tests in simpler form.

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

This looks good to me.  Thanks.
Attachment #8892298 - Flags: review?(ttromey) → review+
Comment on attachment 8892299 [details] [diff] [review]
Make SpiderMonkey Debugger tests pass even though async generators are Nightly-only.

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

Thank you.  This looks good to me.  I found one nit, and also one spot where I don't know the correct thing to do.

::: js/src/frontend/Parser.h
@@ +834,5 @@
>                 pc->isLegacyGenerator();
>      }
>  
>      bool asyncIterationSupported() {
> +        const char* ais = getenv("ASYNC_ITERATION_SUPPORTED");

The purpose of this wasn't clear to me.  Is it so you can test that the tests work?  Does it need to be left in?

I don't know whether there are rules about checking environment variables.  Grepping shows that most variables in js start with "JS_", but it's not universal.

Anyway, this was the one thing I was unsure of.  It's not a blocker for me, I imagine you know the rules here better than I do.

::: js/src/jit-test/lib/nightly-only.js
@@ +1,3 @@
> +// Some experimental features are enabled only on nightly builds, and disabled
> +// on beta and release. Tests for these features should not simply disable
> +// themselves on all but nighly builds, because if we neglect to update such

Typo: nighly -> nightly.
Attachment #8892299 - Flags: review?(ttromey) → review+
(In reply to André Bargull from comment #7)
> Created attachment 8892400 [details] [diff] [review]
> bug1386026-jstests-failure.patch
> 
> This is the patch for the issue mentioned in comment #1. 
> 
> The patch is trivial enough it shouldn't require any review.

FWIW it looks fine to me as well.
(In reply to Tom Tromey :tromey from comment #10)
> (In reply to André Bargull from comment #7)
> > Created attachment 8892400 [details] [diff] [review]
> > bug1386026-jstests-failure.patch
> > 
> > This is the patch for the issue mentioned in comment #1. 
> > 
> > The patch is trivial enough it shouldn't require any review.
> 
> FWIW it looks fine to me as well.

Thanks! :-)
(In reply to Tom Tromey :tromey from comment #9)
> >      bool asyncIterationSupported() {
> > +        const char* ais = getenv("ASYNC_ITERATION_SUPPORTED");
> 
> The purpose of this wasn't clear to me.  Is it so you can test that the
> tests work?  Does it need to be left in?
> 
> I don't know whether there are rules about checking environment variables. 
> Grepping shows that most variables in js start with "JS_", but it's not
> universal.

Argh, no, I just included that in the patch by accident. Thanks for pointing it out.
Revised patch, without debugging getenvs.
Attachment #8892299 - Attachment is obsolete: true
... err, after addressing *all* the review comments
Attachment #8892620 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b78fd576bdc8
Put Debugger tests in simpler form. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/3232054e0a93
Make SpiderMonkey Debugger tests pass even though async generators are Nightly-only. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/a559d546208c
Add parentheses to IIFE when testing async generators. r=tromey
Keywords: checkin-needed
See Also: → 1390730
You need to log in before you can comment on or make changes to this bug.