Remove timeouts for eku tests in b2g arm

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cviecco, Assigned: Cykesiopka)

Tracking

unspecified
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
No description provided.
What is this bug about? Please don't file bugs with no description.
Flags: needinfo?(cviecco)
Reporter

Comment 2

5 years ago
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).
Flags: needinfo?(cviecco)
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.
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?
Reporter

Comment 5

5 years ago
(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?
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?
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.
Reporter

Comment 8

5 years ago
So lets split it. It should not be that hard.
Reporter

Updated

5 years ago
Assignee: nobody → cviecco
Assignee

Comment 9

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

Thanks.
Flags: needinfo?(cviecco)
Reporter

Comment 10

5 years ago
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).
Flags: needinfo?(cviecco)
Reporter

Updated

5 years ago
Assignee: cviecco → nobody
Assignee

Comment 11

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

Thanks.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Assignee

Comment 12

5 years ago
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
Attachment #8544850 - Flags: review?(dkeeler)
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.
Attachment #8544850 - Flags: review?(dkeeler) → review+
Assignee

Comment 14

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

Comment 15

5 years ago
+ Switch last uses of |var| to |let| in generated files
Attachment #8544850 - Attachment is obsolete: true
Attachment #8545783 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0e842f11691f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.