770046 - test_ipc.html does not notice failures

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

test_ipc.html overrides TestRunner.log, but fails to override TestRunner.error. This leads to failures not being counted and test runs going green on TBPL even when tests fail. See <https://tbpl.mozilla.org/php/getParsedLog.php?id=13147301&full=1&branch=mozilla-inbound> for an example.

Kyle, please fix ASAP.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=13312476&tree=Mozilla-Inbound

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm going to disable test_ipc.html if this isn't fixed by tomorrow.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Ms2ger from comment #4)
> I'm going to disable test_ipc.html if this isn't fixed by tomorrow.
By attaching a patch and requesting a review from an IndexedDB peer? Great. Otherwise, please don't.

I'm on vacation this week and won't have any time to do this, but the benefit of running these tests and exercising this code on all platforms far outweighs missing some random orange logging for broken child process behavior. We have found plenty of broken stuff form logging just the parent side already.

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/353b4785836a

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c45b6a8dfb

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Then fix it yourself, as bent is too lazy to do his job.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And get your facts in order, bent is a peer.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to :Ms2ger from comment #0)
> test_ipc.html overrides TestRunner.log, but fails to override
> TestRunner.error. This leads to failures not being counted and test runs
> going green on TBPL even when tests fail. See
> <https://tbpl.mozilla.org/php/getParsedLog.
> php?id=13147301&full=1&branch=mozilla-inbound> for an example.
> 
> Kyle, please fix ASAP.

onTestMessage tries to handle this with a regex, but it probably isn't the right regex.  It probably would make sense to transmit both message and error and map them to things more like exact parallels.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Ms2ger from comment #8)
As I tried to explain in comment 5 this is not a very serious issue. It is simply one of many bugs that I need to fix. We're currently catching some, but not all, test failures. You propose that we should disable this test. Doing so would of course mean that we would not catch *any* failures. I don't see how that helps.

I'm currently working on other bugs which are higher priority but I will fix this as soon as I have the time to do so (unless someone else wants to fix it first, of course).

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It's a five-line patch. I don't understand why you want to waste time arguing about the fix rather that doing the work.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Disabled again, since clearly nothing is going to happen otherwise

https://hg.mozilla.org/integration/mozilla-inbound/rev/77e5cdc7af9a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77e5cdc7af9a

Comment 15

[Ed Morley [:emorley]]

([leave open] needs to be in the whiteboard or the merging tool will close the bug)

Comment 16

[Johnny Stenback (:jst)]

(In reply to :Ms2ger from comment #13)
> Disabled again, since clearly nothing is going to happen otherwise
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/77e5cdc7af9a

Ms2ger, you disabled these tests *again* w/o following due process. As many people have already told you, this is on bent's list of things to do very soon. It's just that there are other things ahead if this in his immediate priority queue and disabling these tests isn't going to change that priority queue or help these tests get fixed any faster.

https://hg.mozilla.org/mozilla-central/rev/99fb6977d46c

Comment 17

[Ed Morley [:emorley]]

The politics around this test disabling aside; the weird thing is that when the test was re-enabled in comment 16 - native Android mochitest-3 went permaorange with bug 781837. Do we have any ideas why?

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Aren't the imported tests before indexedDB?  Are you sure that there's correlation here and not just coincidence?

Comment 19

[Ed Morley [:emorley]]

Press down arrow once on:
https://tbpl.mozilla.org/?noignore=1&tree=Mozilla-Inbound&jobname=Android%20Tegra%20250%20mozilla-inbound%20opt%20test%20mochitest-3&rev=2dfbe4b9403b

Same for:
https://tbpl.mozilla.org/?jobname=Android Tegra 250 mozilla-central opt test mochitest-3&noignore=1&rev=2dfbe4b9403b

The second is not so clear visually (you have to ignore the test_event_propagation.html failures), but both seem to confirm that re-enabling test_ipc.html somehow turned bug 781837 permaorange - unless I'm missing something?

Comment 20

[Ed Morley [:emorley]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #18)
> Aren't the imported tests before indexedDB?  Are you sure that there's
> correlation here and not just coincidence?

Yes I am now.

Filed bug 783513.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14744784&tree=Mozilla-Inbound

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14747063&tree=Mozilla-Inbound

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14747236&tree=Firefox

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This fixes the failure case and makes sure that the harness fails if it isn't run in a child process.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 655653 [details] [diff] [review]
Patch, v1

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

Please separate the test_ipc fixes and the test_blob stuff.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28cbe2f75f5d

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28cbe2f75f5d