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

RESOLVED FIXED in Firefox 3.7a1

Status

()

P3
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: mak)

Tracking

Trunk
Firefox 3.7a1
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
wanted1.9.0.x +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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?
(Reporter)

Comment 2

11 years ago
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
Created attachment 304249 [details] [diff] [review]
Disable 'em! [checked in]

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+
(Reporter)

Comment 7

11 years ago
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+

Comment 11

11 years ago
why do these tests fail?

Comment 12

11 years ago
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
(Reporter)

Comment 13

11 years ago
(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

11 years ago
(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? 

Updated

11 years ago
Target Milestone: --- → Firefox 3 beta4
(Reporter)

Comment 15

11 years ago
(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

11 years ago
(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

11 years ago
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.
(Reporter)

Comment 18

11 years ago
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

11 years ago
(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.

Updated

11 years ago
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 → ---
(Reporter)

Comment 21

10 years ago
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.
(Reporter)

Updated

10 years ago
Assignee: sdwilsh → nobody
(Assignee)

Updated

9 years ago
Blocks: 509868
(Assignee)

Comment 22

9 years ago
Created attachment 395576 [details] [diff] [review]
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Comment 23

9 years ago
Created attachment 395594 [details] [diff] [review]
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.
Attachment #395576 - Attachment is obsolete: true
Attachment #395594 - Flags: review?(sdwilsh)
(Reporter)

Comment 24

9 years ago
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+
(Assignee)

Comment 25

9 years ago
http://hg.mozilla.org/mozilla-central/rev/fd8221dfbe63
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
(Assignee)

Comment 26

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/deeb4cc6b313
status1.9.2: --- → beta1-fixed
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.