983641 - Several DOM Apps tests seem to swallow errors

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Logs of DOM Apps tests indicate a few "Got exception:", without details.

http://dxr.mozilla.org/mozilla-central/search?q=%22Got+exception%3A%22&case=true

This is bad for several reasons:
1. if I understand this code correctly, it simply eats errors, even fatal ones;
2. dump() is bad form, we should rather use info() or ok().

Comment 1

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #0)
> Logs of DOM Apps tests indicate a few "Got exception:", without details.
> 
> http://dxr.mozilla.org/mozilla-central/
> search?q=%22Got+exception%3A%22&case=true
> 
> This is bad for several reasons:
> 1. if I understand this code correctly, it simply eats errors, even fatal
> ones;
> 2. dump() is bad form, we should rather use info() or ok().

We should fix this, but I think that when an exception occurs the tests don't call |finish| and so they fail because of timeouts.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Based on a quick read of a recent log, I have the impression that this is not the case.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch

With this patch we explicitly fail these tests in case of exceptions instead of letting them fail for timeouts.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8391181 [details] [diff] [review]
Patch

Why is it useful to print StopIteration?

Comment 5

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #2)
> Based on a quick read of a recent log, I have the impression that this is
> not the case.

If you throw inside a generator, the next call to next() should throw StopIteration.
For example:
function generator() {
  yield 0;

  referror

  yield 1;
}

var gen = generator();

gen.next() => 0
gen.next() => Reference Error: referror is not defined
gen.next() => StopIteration

So the code after |referror| doesn't get executed.

Anyway it's quite subtle, so it's better to explicitly fail.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #4)
> Comment on attachment 8391181 [details] [diff] [review]
> Patch
> 
> Why is it useful to print StopIteration?

No particular reason, I've left it just because it doesn't do any harm.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #4)
> > Comment on attachment 8391181 [details] [diff] [review]
> > Patch
> > 
> > Why is it useful to print StopIteration?
> 
> No particular reason, I've left it just because it doesn't do any harm.

Well, it adds noise to the logs and it made me think that there was an error :)
I believe that you should remove it.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

I think this is the cleanest way:
- Remove the redundant messages ("Got exception: Stop iteration" and "All done" are useless).
- Catch only StopIteration exceptions, letting the test infrastructure handle other exceptions.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8391755 [details] [diff] [review]
Patch

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

lgtm, but push to try before landing.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6f7723ec6009

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f7723ec6009