989485 - Remove timeouts for eku tests in b2g arm

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

What is this bug about? Please don't file bugs with no description.

Comment 2

[Camilo Viecco (:cviecco)]

The new EKU testing, (due to combinatorics) can take a very long time to run. Even with extra timeout option the tests where still timing out in arm b2g. This bug is to remind us to find another way to disable timeouts (there are some hints in filesystem tests).

Comment 3

[Geoff Brown [:gbrown]]

I am seeing the same problem on Android 2.3, currently only running on Ash but coming to trunk trees soon:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37004620&tree=Ash&full=1

I will update the test manifest to skip on Android also.

Comment 4

[Geoff Brown [:gbrown]]

I investigated the Android failure and found there is effectively a 5 minute limit for each xpcshell test on Android. Setting requesttimeoutfactor extends one timeout in the test harness, but does not affect another, the timeout for devicemanager.shell(). The dm.shell timeout can be increased based on requesttimeoutfactor, but 5 minute+ tests *still* fail because of a timeout in the sutagent, which is more difficult to modify. Could this test be split into 2 parts / 2 separate tests?

Comment 5

[Camilo Viecco (:cviecco)]

(In reply to Geoff Brown [:gbrown] from comment #4)
> I investigated the Android failure and found there is effectively a 5 minute
> limit for each xpcshell test on Android. Setting requesttimeoutfactor
> extends one timeout in the test harness, but does not affect another, the
> timeout for devicemanager.shell(). The dm.shell timeout can be increased
> based on requesttimeoutfactor, but 5 minute+ tests *still* fail because of a
> timeout in the sutagent, which is more difficult to modify. Could this test
> be split into 2 parts / 2 separate tests?

I was actually thinking of making the test 'randomized' in mobile/slow platforms.
Since the test should be the same in all platforms I think it would be OK to
say run 1/4 of the tests on slow devices, with their selection done at runtime.
Random by choosing some seed (outputed) so that on failure we could reproduce the issue.

Do you agree on this plan?

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I don't think that's a worthwhile approach. It should be fairly straightforward to split the test into n pieces and run them all, right?

Comment 7

[Geoff Brown [:gbrown]]

Splitting seems simpler than random.

Also, I think the randomized approach might cause confusion for sheriffing and make it harder to find a regression range in some cases, especially for someone who isn't familiar with the test or isn't aware of the randomness.

Comment 8

[Camilo Viecco (:cviecco)]

So lets split it. It should not be that hard.

Comment 9

[:Cykesiopka]

cviecco: Are you still working on this bug? I can work on it if you aren't.

Thanks.

Comment 10

[Camilo Viecco (:cviecco)]

please(In reply to Cykesiopka from comment #9)
> cviecco: Are you still working on this bug? I can work on it if you aren't.
> 
> Thanks.

Please take it (I am removing myself from the assigned user).

Comment 11

[:Cykesiopka]

(In reply to Camilo Viecco (:cviecco) from comment #10)
> Please take it (I am removing myself from the assigned user).

Thanks.

Comment 12

[:Cykesiopka]

Attachment: bug989485_split-test_cert_eku.js_v1.patch

This seems to be the most straightforward approach.

This patch splits the 3197 tests in test_cert_eku.js into 23 files of 197 tests each, and also removes some certs that aren't generated or used anywhere.

Recent try push:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=61e674c352a6
More comprehensive try push from a few weeks back:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bd31ab5c9fb

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8544850 [details] [diff] [review]
bug989485_split-test_cert_eku.js_v1.patch

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

This looks great! r=me with nit addressed.

::: security/manager/ssl/tests/unit/test_cert_eku-NS_TS.js
@@ +15,5 @@
> +  return certdb.constructX509(der, der.length);
> +}
> +
> +function load_cert(cert_name, trust_string) {
> +  var cert_filename = cert_name + ".der";

nit: let's change this 'var' to 'let' while we're here.

Comment 14

[:Cykesiopka]

(In reply to Cykesiopka from comment #12)
> This patch splits the 3197 tests in test_cert_eku.js into 23 files of 197
> tests each

My brain seems to have failed here... I meant 23 files of 139 tests each.

Comment 15

[:Cykesiopka]

Attachment: bug989485_split-test_cert_eku.js_v2.patch

+ Switch last uses of |var| to |let| in generated files

Comment 16

[:Cykesiopka]

Thanks for the review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53661181f4a6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e842f11691f

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/0e842f11691f