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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Also the jsreftest failure from below, which is from bug 1379717 I believe:
https://treeherder.mozilla.org/logviewer.html#?job_id=119721267&repo=try

Comment 2

[André Bargull [:anba]]

(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... :-/

Comment 3

[Jim Blandy :jimb]

Attachment: Make SpiderMonkey Debugger tests pass even though async generators are Nightly-only.

Comment 4

[Jim Blandy :jimb]

Attachment: Put Debugger tests in simpler form.

Comment 5

[Jim Blandy :jimb]

Attachment: Make SpiderMonkey Debugger tests pass even though async generators are Nightly-only.

Comment 6

[Jim Blandy :jimb]

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.

Comment 7

[André Bargull [:anba]]

Attachment: 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.

Comment 8

[Tom Tromey :tromey]

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.

Comment 9

[Tom Tromey :tromey]

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.

Comment 10

[Tom Tromey :tromey]

(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.

Comment 11

[André Bargull [:anba]]

(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! :-)

Comment 12

[Jim Blandy :jimb]

(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.

Comment 13

[Jim Blandy :jimb]

Attachment: Make SpiderMonkey Debugger tests pass even though async generators are Nightly-only. r=tromey

Revised patch, without debugging getenvs.

Comment 14

[Jim Blandy :jimb]

Attachment: Make SpiderMonkey Debugger tests pass even though async generators are Nightly-only. r=tromey

... err, after addressing *all* the review comments

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b78fd576bdc8
https://hg.mozilla.org/mozilla-central/rev/3232054e0a93
https://hg.mozilla.org/mozilla-central/rev/a559d546208c

Comment 17

[Intermittent Failures Robot]

71 failures in 888 pushes (0.08 failures/push) were associated with this bug in the last 7 days. 

This is the #15 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* try: 71

Platform breakdown:
* linux64: 18
* windows10-64: 10
* linux32: 7
* windows7-32: 6
* linux64-qr: 6
* windows7-32-nightly: 3
* windows10-64-nightly: 3
* linux64-nightly: 3
* linux64-devedition: 3
* linux32-nightly: 3
* linux32-devedition: 3
* windowsxp: 2
* windows8-64: 2
* android-4-3-armv7-api15: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1386026&startday=2017-07-31&endday=2017-08-06&tree=all