Closed Bug 416066 Opened 13 years ago Closed 12 years ago

Tests for bug 399606 are commented out and unreliable, so fix them

Categories

(Firefox :: Bookmarks & History, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: sdwilsh, Assigned: mak)

References

Details

Attachments

(2 files, 1 obsolete file)

I need to fix them...

They are also named wrong!  yay!
Please disable browser_bug413985-httprefresh.js as it has failed on two totally unrelated checkins, bug 392322 and bug 417124, both of them blocking1.9+.
Blocks: 392322, 417124
Flags: blocking-firefox3?
someone else should do them as I have no cycles for roughly a week still.
Dietrich: can you get someone else in the Places team to disable these tests?
Assignee: sdwilsh → dietrich
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
Assignee: dietrich → mak77
This kills them for the moment.
Attachment #304249 - Flags: review?(dietrich)
(In reply to comment #4)
> Created an attachment (id=304249) [details]
> Disable 'em!
> 
> This kills them for the moment.
> 

if only the one test fails, i'd rather not disable them all.
Comment on attachment 304249 [details] [diff] [review]
Disable 'em! [checked in]

clarified over irc: all the tests are causing problems.
Attachment #304249 - Flags: review?(dietrich) → review+
All the tests test the same way, so they all have the same probability to fail :(

testing non-deterministic behavior FTL :(
reassigning per irc: shawn's going to look into a way of getting these to work w/o re-engineering how we add visits, and if that's not possible then maybe just converting these into litmus tests.
Assignee: mak77 → sdwilsh
Comment on attachment 304249 [details] [diff] [review]
Disable 'em! [checked in]

Disabled those tests.
Attachment #304249 - Attachment description: Disable 'em! → Disable 'em! [checked in]
Removing blocking-firefox3+ now that the tests are disabled, re-nom if this really should block.
Flags: blocking-firefox3+
why do these tests fail?
Don't remove a blocking flag when you disable broken tests. Fix the tests.
Flags: blocking-firefox3+
Summary: Tests for bug 399606 are commented out and unreliable → Tests for bug 399606 are commented out and unreliable, so fix them
(In reply to comment #11)
> why do these tests fail?
The problem is that the back-end code uses an nsITimer after pageload to add something to history.  We have to check that two page loads do not cause two observer notifications to be sent.  This works great locally, but seems to run waaay slower on the test boxes, causing them to fail sometimes. I've mulled over how to fix the tests, but I have yet to come up with a way to work around this.  Suggestions welcome.
(In reply to comment #13)
> (In reply to comment #11)
> > why do these tests fail?
> The problem is that the back-end code uses an nsITimer after pageload to add
> something to history.

Why can't you poll instead of waiting for a set time? 

Target Milestone: --- → Firefox 3 beta4
(In reply to comment #14)
> Why can't you poll instead of waiting for a set time? 
are you referring to the test or the code?  If it's the code, I can't really comment on that as I didn't work on the feature, nor was I involved with the decision that lead to the use of the timer.  If it's the test, it wouldn't matter because we'd still have to wait for the observer notification before the entry would be in the database.
(In reply to comment #15)
> 
> If it's the test

I am asking about the test. Maybe I have misunderstood.

What is the specific failure in the test? This is not a satisfactory explanation:

"testing non-deterministic behavior FTL :("
OK, I ran these in a slow environment, and I agree they will not work as written. I don't think that makes these bugs untestable though.
I guess comment 16 doesn't need to be addressed then?  I agree that they should be testable, but I have yet to come up with something.  Suggestions welcome (I'll even implement them - I just have run out of ideas for the time being).
(In reply to comment #18)
> I guess comment 16 doesn't need to be addressed then?  

Incorrect.

> I agree that they should
> be testable, but I have yet to come up with something.  Suggestions welcome

The people checking in code should know how test their own code.

Priority: P1 → P3
Target Milestone: Firefox 3 beta4 → Firefox 3
We need to revisit this when the smoke clears, as part of the larger "Places is poorly tested" issue we're seeing.
Flags: wanted1.9.0.x+
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Target Milestone: Firefox 3 → ---
So, we could be a bit evil here and keep the observers registered until quit-applicaiton comes around, and then unregister them.  If we don't get a second callback by then, we should be OK.
Assignee: sdwilsh → nobody
Blocks: 509868
Attached patch patch v1.0 (obsolete) — Splinter Review
i have to read this a second time, but should be ok.
i merged all the tests into one and moved support files into a subfolder.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1.1Splinter Review
the only drawback of this test is that will be a bit "slow", i have to wait at least 2*LAZY_ADD_TIMER to make the test valid. Since tests are run parallel should be less an issue today.
In case we don't want to take the hit, i'd still would like to take the patch to unify the tests, without enabling the new one.
Attachment #395576 - Attachment is obsolete: true
Attachment #395594 - Flags: review?(sdwilsh)
Comment on attachment 395594 [details] [diff] [review]
patch v1.1

Use XPCOMUtils.generateQI instead of rolling your own please.

r=sdwilsh
Attachment #395594 - Flags: review?(sdwilsh) → review+
http://hg.mozilla.org/mozilla-central/rev/fd8221dfbe63
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
No longer blocks: 535905
Depends on: 535905
You need to log in before you can comment on or make changes to this bug.