849239 - Remove leftover caught exception print()s in js tests, to avoid TBPL false positives

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ed Morley [:emorley]]

Broken out from bug 829684.

(In reply to Ed Morley [:edmorley UTC+0] from comment #0)
> Bug 816971 is attempting to improve TBPL's ability to show Python exceptions
> in it's annotated summary.
> 
> However, testing the patch there showed that jsreftests output many
> (presumably) expected JS exceptions to stdout, which match the regex in that
> bug, and would cause confusion if we landed this on TBPL production.
> 
> There were 30+ such false positives in a fedora opt jsreftest run - some
> examples:
> ...

(In reply to Terrence Cole [:terrence] from comment #2)
> That sounds reasonable, however, for the two extant tests that you showed,
> there is no reason for them to be printing at all: the prints look like
> debugging code that should have been removed. If there are only ~30 of these
> tests, it would probably be easier to just fix the tests.

Comment 1

[Ed Morley [:emorley]]

Attachment: Patch v1

https://tbpl.mozilla.org/?tree=Try&rev=48d98e6353be

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #1)
> Created attachment 722793 [details] [diff] [review]
> Patch v1
> 
> https://tbpl.mozilla.org/?tree=Try&rev=48d98e6353be

Would it make sense to replace it by a "log" function?

Comment 3

[Terrence Cole [:terrence]]

Do we have a log function?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Terrence Cole [:terrence] from comment #3)
> Do we have a log function?

I don't know, maybe we can use printErr or console.log for implementing it.

Comment 5

[Ed Morley [:emorley]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Would it make sense to replace it by a "log" function?

In what sense? As a more thorough version of bug 829684?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Ed Morley (Away 29th-1st, UK public holiday) [:edmorley UTC+0] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > Would it make sense to replace it by a "log" function?
> 
> In what sense? As a more thorough version of bug 829684?

I don't really care about the way it is formatted and I don't think this is good to have it in tbpl, but it would be nice to have it when tests are run individually.

These logs are usually useful indicators of the error that we expect, and prevent a few minutes of questioning and instrumentation of tests cases when a test is failing.  I would be a bit sad if we had to remove them.

Comment 7

[Ed Morley [:emorley]]

Ah ok, so you're saying have a logIfNotTBPL()?

I only filed this due to what terrence said (comment 0), so in which case we might as well WONTFIX this is preference of bug 829684, which will mean the additional log output is present in both local and TBPL logs (which seems more useful than just having in local logs), but yet avoids the false positives with the parsing.

Comment 8

[Terrence Cole [:terrence]]

(In reply to Ed Morley (Away 29th-1st, UK public holiday) [:edmorley UTC+0] from comment #7)
> I only filed this due to what terrence said (comment 0), so in which case we
> might as well WONTFIX this is preference of bug 829684, which will mean the
> additional log output is present in both local and TBPL logs (which seems
> more useful than just having in local logs), but yet avoids the false
> positives with the parsing.

For what it is worth, I strongly disagree with Nicolas on this. If these prints are making your life harder with every single push, then keeping them for the off chance that they may be useful to someone, someday makes no sense whatsoever.

I see it like this: it may save someone ~5min to read and add the prints, however, it only in practice saves time if it actually happens. The expected time savings of these prints is the probabilistic function: E(savings) = 5min * P(test failure caught by print). We have 10,000+ tests that run on every push and we're talking about removing ~30 prints. The probability that one of those prints is going to be helpful is extremely close to zero. This means that our expect time savings are also effectively zero.

Thus, even if Ed is only shaving off microseconds of his per-push workflow by removing these prints, then we should still expect positive time savings. We've probably already spent more time discussing this nit than going with a "better" solution could possibly save.

Ultimately, I don't really care which solution you go with, but I still suggest you just delete these prints: they are in your way, they aren't being actively used by anyone, and they are trivial to re-add if needed.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Terrence Cole [:terrence] from comment #8)
> (In reply to Ed Morley (Away 29th-1st, UK public holiday) [:edmorley UTC+0]
> from comment #7)
> > I only filed this due to what terrence said (comment 0), so in which case we
> > might as well WONTFIX this is preference of bug 829684, which will mean the
> > additional log output is present in both local and TBPL logs (which seems
> > more useful than just having in local logs), but yet avoids the false
> > positives with the parsing.
> 
> I see it like this: it may save someone ~5min to read and add the prints,
> however, it only in practice saves time if it actually happens. The expected
> time savings of these prints is the probabilistic function: E(savings) =
> 5min * P(test failure caught by print). We have 10,000+ tests that run on
> every push and we're talking about removing ~30 prints. The probability that
> one of those prints is going to be helpful is extremely close to zero. This
> means that our expect time savings are also effectively zero.

I am ok with this argument, sorry for holding the patch.

Comment 10

[Ed Morley [:emorley]]

Comment on attachment 722793 [details] [diff] [review]
Patch v1

> I am ok with this argument, sorry for holding the patch.

No problem :-)

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 722793 [details] [diff] [review]
Patch v1

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

Looks good!

Comment 12

[Ed Morley [:emorley]]

Thank you :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/4063723a2c4b

Comment 13

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/4063723a2c4b