Closed Bug 989485 Opened 8 years ago Closed 7 years ago

Remove timeouts for eku tests in b2g arm

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: cviecco, Assigned: Cykesiopka)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
What is this bug about? Please don't file bugs with no description.
Flags: needinfo?(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).
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.
Blocks: 979921
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?
(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.
So lets split it. It should not be that hard.
Assignee: nobody → cviecco
cviecco: Are you still working on this bug? I can work on it if you aren't.

Thanks.
Flags: needinfo?(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).
Flags: needinfo?(cviecco)
Assignee: cviecco → nobody
(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
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+
(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.
+ 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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.