Closed Bug 719954 Opened 8 years ago Closed 8 years ago

Robotium testBookmark hangs

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 12

People

(Reporter: kats, Assigned: kats)

Details

(Whiteboard: not-fennec-11)

Attachments

(2 files)

Two problems.
1) testBookmark has nothing to do with bookmarks, it actually loads a random history entry. In my case, it loaded a webm video which I don't think fires a DOMContentLoaded event. Or if it does, the event was missed because of...
2) ... a race condition that exists in the test. If the page loads really quickly after clicking on the item, then it is possible that we start listening for the event after it has been fired.

Patches coming shortly.
Comment on attachment 590294 [details] [diff] [review]
(2/2) Protect against race

Review of attachment 590294 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, clearly necessary. I must be more careful about introducing this sort of bug.
Attachment #590294 - Flags: review?(gbrown) → review+
Comment on attachment 590293 [details] [diff] [review]
(1/2) Load a bookmark instead of a history item

Review of attachment 590293 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that testBookmark should be testing bookmarks rather than history, and this is a straight forward way to do that: I think it is a good change. 

I am not clear on the history of the test: why was it done this way in the first place? Requesting additional review from :jmaher just in case he has concerns / insight into the original intent.
Attachment #590293 - Flags: review?(jmaher)
Attachment #590293 - Flags: review?(gbrown)
Attachment #590293 - Flags: review+
Comment on attachment 590293 [details] [diff] [review]
(1/2) Load a bookmark instead of a history item

Review of attachment 590293 [details] [diff] [review]:
-----------------------------------------------------------------

these tests were written more as examples of navigating stuff.  As we hacked away at making this work, then cleaned it up over and over again the context of what we were testing was lost.  Hence the improper test case.  

I would say now that we have tools in place for testing, we should have somebody sit down and really outline what we want to test for each area instead of assuming what we have is good.  My instincts tell me what we have is a good starting place for each area and with some extending of the existing cases we can have a much more comprehensive set of tests.
Attachment #590293 - Flags: review?(jmaher) → review+
Whiteboard: not-fennec-11
You need to log in before you can comment on or make changes to this bug.