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

Status: RESOLVED FIXED

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

Description

[Shawn Wilsher :sdwilsh]

I need to fix them...

They are also named wrong!  yay!

Comment 1

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

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+.

Comment 2

[Shawn Wilsher :sdwilsh]

someone else should do them as I have no cycles for roughly a week still.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Dietrich: can you get someone else in the Places team to disable these tests?

Comment 4

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

Attachment: Disable 'em! [checked in]

This kills them for the moment.

Comment 5

[Dietrich Ayala (: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 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 304249 [details] [diff] [review]
Disable 'em! [checked in]

clarified over irc: all the tests are causing problems.

Comment 7

[Shawn Wilsher :sdwilsh]

All the tests test the same way, so they all have the same probability to fail :(

testing non-deterministic behavior FTL :(

Comment 8

[Dietrich Ayala (:dietrich)]

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.

Comment 9

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

Comment on attachment 304249 [details] [diff] [review]
Disable 'em! [checked in]

Disabled those tests.

Comment 10

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

Removing blocking-firefox3+ now that the tests are disabled, re-nom if this really should block.

Comment 11

[Robert Sayre]

why do these tests fail?

Comment 12

[Robert Sayre]

Don't remove a blocking flag when you disable broken tests. Fix the tests.

Comment 13

[Shawn Wilsher :sdwilsh]

(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.

Comment 14

[Robert Sayre]

(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? 

Comment 15

[Shawn Wilsher :sdwilsh]

(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.

Comment 16

[Robert Sayre]

(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 :("

Comment 17

[Robert Sayre]

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.

Comment 18

[Shawn Wilsher :sdwilsh]

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).

Comment 19

[Robert Sayre]

(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.

Comment 20

[Mike Connor [:mconnor]]

We need to revisit this when the smoke clears, as part of the larger "Places is poorly tested" issue we're seeing.

Comment 21

[Shawn Wilsher :sdwilsh]

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.

Comment 22

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 395594 [details] [diff] [review]
patch v1.1

Use XPCOMUtils.generateQI instead of rolling your own please.

r=sdwilsh

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/fd8221dfbe63

Comment 26

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/deeb4cc6b313

Comment 27

[Gervase Markham [:gerv]]

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