Closed Bug 805712 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: philor, Assigned: baku)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

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!
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...
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().
Attached patch patch 1 (obsolete) — Splinter Review
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+
Attached patch patch b (obsolete) — Splinter Review
Attachment #676147 - Attachment is obsolete: true
Attachment #676161 - Flags: review+
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
Attached patch patch cSplinter Review
Attachment #676161 - Attachment is obsolete: true
Attachment #676532 - Flags: review+
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-
(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".
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Andrea, can you please request Aurora approval on this?
Actually, this is test-only. I'll land it.
Whiteboard: [orange]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.