Closed Bug 1242832 Opened 7 years ago Closed 7 years ago

Intermittent browser_storage_overflow.js | This test exceeded the timeout threshold. It should be rewritten or split up.

Categories

(DevTools :: Storage Inspector, defect)

Product:
DevTools
Component:
Storage Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 unaffected, firefox46 fixed, firefox47 fixed, firefox-esr45 unaffected)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed
firefox47 --- fixed
firefox-esr45 --- unaffected

People

(Reporter: philor, Assigned: ntim)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Patch
1.58 KB, patch
pbro
: review+
Details | Diff | Splinter Review
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Blocks: 1171903
Attached patch Patch — — Splinter Review 
The right fix would be to optimize the CSS further, but it's unclear what I can still optimize here.

Reducing the number of items will also reduce the number of CSS reflows, so it should surely help.
Attachment #8724418 - Flags: review?(pbrosset)
See bug 1171903 for more context on the failure.
Comment on attachment 8724418 [details] [diff] [review]
Patch

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

I'm R+ this because I don't see any problem with changing 200 to 175. So in itself, this patch looks fine.

However, I spent some time thinking about the overall story for this test.
I don't fully understand what's causing this test to timeout. Do you know what takes the most time?
Sure it times out on linux debug (which is our slowest test environment), but still, it doesn't do all that much.

If what takes time is the client-server round-trips to get the additional data, then we should mock that out. This test isn't concerned with making sure data can be retrieved from a server, this is already tested in numerous other places. So if it turns out that sending requests and waiting for responses is what takes the most time, then that could be fixed by overriding fetchStorageObjects in gUI. Or creating a custom storageType that's client-side only.

If, instead, what takes time is the scroll event simulation, then we can avoid it by calling `gUI.handleScrollEnd()` instead of `table.scrollTop += cellHeight * 50` in the test. Here again, this test isn't concerned with testing the scroll event listener logic of TableWidget.js (I'm assuming this is tested in devtoos/client/shared/test/ already).
Locally for me, the test takes 12s and with this fixed, it takes 9s. Worth giving it a try?

You're saying that what takes the most time is CSS reflows? So, just creating the extra markup for the new rows laying them out and painting them is slow?
If that is the bottleneck, then I agree we should try and find ways to make it faster, but only if it's a noticeable problem for users.

Last, this is typically the type of test where `requestLongerTimeout(2)` is fine to use. There's no sense in splitting it in 2, and it can't really be rewritten to be simpler. So maybe just increase the timeout.
Additionally, why not make the table be 100 rows only, and scrolling less often (by the way, the itemOffset of 50 in handleScrollEnd is a "magic number", this should be a const at the top of file, and ideally it should be exported so that the test can change it to something smaller).
Attachment #8724418 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/6af962bff65d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.