Intermittent test_archivereader_zip_in_zip.html | ArchiveReader + FileReader should work! (probably not being counted as a failure)

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: philor, Assigned: baku)

Tracking

({intermittent-failure})

Trunk
mozilla19
x86
Windows XP
intermittent-failure
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I'm pretty sure I've seen this failure before (these failures, it seems to always be two with the same message), mixed in with other failures where it looked like it could just be the result of some other test having caused a cascade of fail.

In this case, as you can see from https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a09013468b3e&jobname=Rev3%20WINNT%205.1%20mozilla-inbound%20debug%20test%20mochitests-2/5 (click the orange 2, at the bottom of the page you're told "mochitest-plain-2: 216924/0/18540 L-FAIL" when it should be "216924/2/18540" for the two failures), they are not being counted as failures, and thus they may well be happening a totally unknowable number of times in runs that are reported as being green.

The one situation I can remember that has caused that before was a test which thought it was hitting the backspace key in an iframe, but it really navigated the whole mochitest harness window Back instead (which as I remember it jgriffin discovered from the ateam's logparser).

https://tbpl.mozilla.org/php/getParsedLog.php?id=16462389&tree=Mozilla-Inbound
Rev3 WINNT 5.1 mozilla-inbound debug test mochitests-2/5 on 2012-10-25 10:44:54 PDT for push a09013468b3e
slave: talos-r3-xp-066

2220 INFO TEST-START | /tests/dom/file/test/test_archivereader_zip_in_zip.html
2221 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_archivereader_zip_in_zip.html | ArchiveReader + FileReader should work!
2222 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_archivereader_zip_in_zip.html | ArchiveReader + FileReader should work!
(Assignee)

Comment 1

5 years ago
What I don't understand is that the error message "ArchiveReader + FileReader should work!" is not part of test_archivereader_zip_in_zip.html but test_archivereader.html...
Comment hidden (Treeherder Robot)
(Reporter)

Comment 3

5 years ago
That's easy enough to arrange: just continue running after calling SimpleTest.finish(), and your results will be reported as having come from the next test. Apparently test_archivereader.html is neither finished, nor stopped, at the point it calls finish().
Comment hidden (Treeherder Robot)
(Assignee)

Comment 5

5 years ago
Created attachment 676147 [details] [diff] [review]
patch 1

This patch implements a better state-machine for the mochitest
Attachment #676147 - Flags: review?(mounir)
Comment on attachment 676147 [details] [diff] [review]
patch 1

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

r=me with the comment applied.

::: dom/file/test/test_archivereader.html
@@ +54,5 @@
>      return fileList.files[0];
>    }
>  
> +  handleFinished = 0;
> +  function handleFinish() {

I would prefer this to be splitted in two methods:
function markTestDone() {
  ++handleFinished;
}
function isFinished() {
  return handleFinished == 5;
}

so you would do:
markTestDone();
if (isFinished()) {
  SimpleTest.finish();
}

I know it's more verbose but "handleFinish" wasn't understandable, I had to read the implementation.
Attachment #676147 - Flags: review?(mounir) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 676161 [details] [diff] [review]
patch b
Attachment #676147 - Attachment is obsolete: true
Attachment #676161 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Andrea: Looks like you didn't address Mounir's review-comment.  The new patch is identical to the old patch, except for the addition of a commit message.

So, removing "checkin-needed", since the latest patch isn't yet ready to land.
Keywords: checkin-needed
Assignee: nobody → amarchesini
Status: NEW → ASSIGNED
(Assignee)

Comment 9

5 years ago
Created attachment 676532 [details] [diff] [review]
patch c
Attachment #676161 - Attachment is obsolete: true
Attachment #676532 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Comment on attachment 676532 [details] [diff] [review]
patch c

Andrea, it seems that this patch doesn't fully address my comments. It's not a big deal but it is breaking an implicit rule we have when we set "r+ with those comments addressed". When this happen, the reviewer expect the patch author to apply all comments or engage a conversation when there is a disagreement.
Pushing a patch without addressing comments or not addressing them entirely isn't considered a bad practice.
Attachment #676532 - Flags: review+ → review-
Keywords: checkin-needed
(In reply to Mounir Lamouri (:mounir) from comment #10)
> Pushing a patch without addressing comments or not addressing them entirely
> isn't considered a bad practice.

Sorry, I obviously meant "*is* considered a bad practice".
(Assignee)

Comment 12

5 years ago
Mounir, you wrote: "I would prefer this to be splitted in two methods [...] I know it's more verbose but "handleFinish" wasn't understandable".

I use that 'handleFinish' 12 times. I don't want to have the same code written 12 times.
This is the reason why I changed your 'markTestDone()' in order to do what you suggest to duplicate many times just once.
Comment on attachment 676532 [details] [diff] [review]
patch c

Yeah, actually the r- was stupid. I just wanted to underline the fact that it's very important to take into account all comments (event nits) and make sure you explain why you don't if you don't.
I should stop reading bugmail while listening to people, my brain isn't able to do that and I do stupid things :)

I will push that for you ;)
Attachment #676532 - Flags: review-
https://hg.mozilla.org/mozilla-central/rev/2183064c4b7b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Andrea, can you please request Aurora approval on this?
Comment hidden (Treeherder Robot)
Actually, this is test-only. I'll land it.
https://hg.mozilla.org/releases/mozilla-aurora/rev/423ec29c585b
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.