894729 - test_0030_general.js and test_0082_prompt_unsupportAlreadyNotified.js tests cannot be run concurrently

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Spinoff of bug 889183.

These two tests use a hardcoded http port.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

I'll push to try after try reopens

Comment 3

[Robert Strong (they/them - no direct email)]

Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=ff2e629f259f

Comment 4

[Robert Strong (they/them - no direct email)]

Brian, test_0030_general.js takes quite a lot longer than other tests and I've noticed that when running the tests in parallel several other tests end up taking the same amount of time as test_0030_general.js (around 28 seconds + or - several seconds). I'm considering splitting test_0030_general.js out into individual tests. What do you think?

Comment 5

[Robert Strong (they/them - no direct email)]

Weird
 0:36.18 TEST-PASS | test_0010_general.js | test passed (time: 35782.000ms)
 0:36.18 TEST-PASS | test_0020_general.js | test passed (time: 35779.000ms)
 0:36.18 TEST-PASS | test_0060_manager.js | test passed (time: 35773.000ms)
 0:36.18 TEST-PASS | test_0050_general.js | test passed (time: 35775.000ms)
 0:36.19 TEST-PASS | test_0030_general.js | test passed (time: 35790.000ms)
 0:36.19 TEST-PASS | test_0040_general.js | test passed (time: 35784.000ms)

All other tests appear not to be affected in the same way

Comment 6

[Robert Strong (they/them - no direct email)]

Try run times... they aren't grouped together either. weird

18:59:38 INFO - TEST-PASS | test_0010_general.js | test passed (time: 406.000ms)
18:59:39 INFO - TEST-PASS | test_0020_general.js | test passed (time: 1109.000ms)
18:59:39 INFO - TEST-PASS | test_0040_general.js | test passed (time: 891.000ms)
18:59:39 INFO - TEST-PASS | test_0050_general.js | test passed (time: 1078.000ms)
18:59:39 INFO - TEST-PASS | test_0060_manager.js | test passed (time: 906.000ms)
18:59:52 INFO - TEST-PASS | test_0030_general.js | test passed (time: 14467.000ms)

Comment 7

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #4)
> Brian, test_0030_general.js takes quite a lot longer than other tests and
> I've noticed that when running the tests in parallel several other tests end
> up taking the same amount of time as test_0030_general.js (around 28 seconds
> + or - several seconds). I'm considering splitting test_0030_general.js out
> into individual tests. What do you think?

That's fine by me but maybe we could instead find out why it's slow and fix that.

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #4)
> > Brian, test_0030_general.js takes quite a lot longer than other tests and
> > I've noticed that when running the tests in parallel several other tests end
> > up taking the same amount of time as test_0030_general.js (around 28 seconds
> > + or - several seconds). I'm considering splitting test_0030_general.js out
> > into individual tests. What do you think?
> 
> That's fine by me but maybe we could instead find out why it's slow and fix
> that.
I am very certain it has to do with the test using nsIIncrementalDownload. I've added mock implementations just about everywhere else along with adding tests for the network component so it is still tested to speed up the app update tests and nsIIncrementalDownload is the one remaining component to do this with.

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 812361 [details] [diff] [review]
patch rev1

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

nice :)

Comment 10

[Brian R. Bondy [:bbondy]]

we're using a mock impementation of nsIIncrementalDownload there too though, so I'd think it would be fast.  Did you check the distribution of the time spent in the log file to see if it's evenly distributed across all of the test parts?
Or does run_test_pt13 take a long time and everything else is fast?

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> we're using a mock impementation of nsIIncrementalDownload there too though,
> so I'd think it would be fast.  Did you check the distribution of the time
> spent in the log file to see if it's evenly distributed across all of the
> test parts?
> Or does run_test_pt13 take a long time and everything else is fast?
I haven't checked yet but I am looking at it right now.

I am fairly certain that the mock implementation is fast... keep in mind that the mock nsIIncrementalDownload isn't used until the 13th test! I am certain that test 1 thru 12 that are slow since this test has always taken a long time from my experience before you added test 13.

Comment 12

[Brian R. Bondy [:bbondy]]

In that case a blind fix of moving the mock implementation init up might fix it.  That removes all tests that involve the real incremental downloader, but app update isn't really the place it should be tested.

Comment 13

[Brian R. Bondy [:bbondy]]

Maybe some additional code is needed to make the mock implementation more generic though, I don't recall without looking at the code.

Comment 14

[Robert Strong (they/them - no direct email)]

That's the long term plan but with xpcshell running in parallel I'd like to split them anyways since that should lessen the time overall as well and I don't want to do that until after there are nsIIncrementalDownload tests.

Comment 15

[Brian R. Bondy [:bbondy]]

Yep, that sounds like a good plan.  Please file a follow up for using the mock earlier for the future dependent on bug 886061.

Comment 16

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/4fe6e345e0b0

Comment 17

[Robert Strong (they/them - no direct email)]

Just checked if using the mock nsIIncrementalDownload made a difference and it didn't. :(

I'll investigate some more

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fe6e345e0b0

Comment 19

[Robert Strong (they/them - no direct email)]

Filed bug 922984 to investigate if the test time to complete can be improved by using a mock nsIIncrementalDownload.

Filed bug 922981 to split test_0030_general.js into individual tests.