Closed Bug 462986 Opened 16 years ago Closed 16 years ago

Fix private browsing tests to pass on tinderbox (session store test)

Categories

(Firefox :: Private Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.1b2

People

(Reporter: sdwilsh, Assigned: ehsan.akhgari)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 1 obsolete file)

After landing bug 248970, one test failed on tinderbox.  I made it a todo, with a comment referencing this bug.  We should understand what the failure is before we release beta 2 in my opinion, but I do not think it was enough to backout the whole feature.

The test in question is browser_248970_a.js in sessionstore.  So far it has failed on Linux and windows.  Still waiting on OS X
Flags: blocking-firefox3.1?
Attached patch Patch (v1) (obsolete) — Splinter Review
OK, so this is basically a timing issue.  If the test is executed quickly enough, then the timestamps will be the same after entering the private browsing mode, even though saveState *has* been called.  A delay as low as 50ms should be enough here, to ensure that the timestamps *will* be different if the file is written on entry, and also to make the test take as little time as possible.

(I'm sorry for how the patch is, couldn't make diff -w work; but anyway the code inside the timer has not changed -- I only removed Shawn's comment.)
Attachment #346213 - Flags: review?(zeniko)
Attachment #346213 - Flags: review?(zeniko) → review+
http://hg.mozilla.org/mozilla-central/rev/fbae114d6133
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I've backed this out due to the test failures on Linux
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Simon: do you think we need a larger timeout here?  I really think that this is a timing issue, but I don't want to set a large timeout value to make tests slower than needed (see bug 463021).
If the writes are too quick in succession, we might have to use a different strategy: Just delete sessionstore.js and verify that it gets recreated (which should equal an updated lastModifiedDate but apparently doesn't).
Attached patch Second trySplinter Review
(In reply to comment #6)
> If the writes are too quick in succession, we might have to use a different
> strategy: Just delete sessionstore.js and verify that it gets recreated (which
> should equal an updated lastModifiedDate but apparently doesn't).

This is a great idea.  Here's the patch which does this.  No more flaky timeouts!  :-)
Attachment #346213 - Attachment is obsolete: true
Attachment #346322 - Flags: review?(zeniko)
Comment on attachment 346322 [details] [diff] [review]
Second try

Sure, just please be somewhat more specific with the variable names: What "path" and what "file"? r+=me with that fixed.
Attachment #346322 - Flags: review?(zeniko) → review+
Attached patch For checkinSplinter Review
I may not be around long enough tonight to check this in myself...
Keywords: checkin-needed
We decided to leave this till after Beta 2's freeze.
Keywords: checkin-needed
Target Milestone: Firefox 3.1b2 → Firefox 3.1
http://hg.mozilla.org/mozilla-central/rev/d6b4f82c604b
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b2
This is still causing some oranges.  Example: <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1226216246.1226219612.30745.gz>

The nature of this failure is exactly like the previous one described in comment 6.  I'll attach a patch with the same fix as the previous patch landed for the failing test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Third trySplinter Review
Attachment #347144 - Flags: review?(zeniko)
Attachment #347144 - Attachment is patch: true
Attachment #347144 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 347144 [details] [diff] [review]
Third try

+  // set the interval to 0

This is pretty much obvious from the code. ;-)
If anything, you rather want to describe the desired effect(s) this has.
Attachment #347144 - Flags: review?(zeniko) → review+
(In reply to comment #15)
> (From update of attachment 347144 [details] [diff] [review])
> +  // set the interval to 0
> 
> This is pretty much obvious from the code. ;-)
> If anything, you rather want to describe the desired effect(s) this has.

Fixed on check-in.

http://hg.mozilla.org/mozilla-central/rev/8b146fe55de9
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Blocks: 463021
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
QA Contact: general → private.browsing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: