Open Bug 1391703 Opened 3 years ago Updated 3 years ago

Introduce common JSM for security/manager/tools/ scripts

Categories

(Core :: Security: PSM, enhancement, P3)

enhancement

Tracking

()

REOPENED
Tracking Status
firefox57 --- unaffected

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

(Depends on 1 open bug)

Details

(Keywords: stale-bug, Whiteboard: [psm-blocked])

Attachments

(1 file)

PSM has various xpcshell scripts under the security/manager/tools/ folder. At the moment, these scripts:
  1. Duplicate code.
  2. Aren't testable.

We should introduce a common, testable JSM that these scripts can use.
Comment on attachment 8899222 [details]
Bug 1391703 - Introduce common JSM for security/manager/tools/ scripts.

https://reviewboard.mozilla.org/r/170484/#review176454

This is a great improvement in our situation - thanks!
I guess we don't really have a way to test if these changes will work in the automation infrastructure, but even if things do break there, it won't break the build (famous last words), so I think we can go ahead with this and just keep an eye on automation when it runs with this.

::: security/manager/moz.build:11
(Diff revision 1)
>  
>  with Files("**"):
>      BUG_COMPONENT = ("Core", "Security: PSM")
>  
> -DIRS += ['ssl', 'locales']
> +DIRS += [
> +  'locales',

nit: 4-space indentation here, if I understand correctly

::: security/manager/tools/tests/test_psmtoolutils.js:47
(Diff revision 1)
> +});
> +
> +add_task(function testDownloadFile_BadStatusCode() {
> +  let url = `${gHttpServerPrePath}/BadStatusCode`;
> +  throws(() => PSMToolUtils.downloadFile(url, true),
> +         /problem downloading.*status/,

Should we also add the expected status code (404)?
Attachment #8899222 - Flags: review?(dkeeler) → review+
Comment on attachment 8899222 [details]
Bug 1391703 - Introduce common JSM for security/manager/tools/ scripts.

https://reviewboard.mozilla.org/r/170484/#review176454

Thanks for the review.
Yeah, it's unfortunate we can't really test this on automation, but oh well.

> nit: 4-space indentation here, if I understand correctly

Yes, you're correct. Fixed.

> Should we also add the expected status code (404)?

Sounds good.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/121e4d470c11
Introduce common JSM for security/manager/tools/ scripts. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/121e4d470c11
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/56188620cce0
Backed out changeset 121e4d470c11 for breaking periodic HSTS/HPKP updates.
Comment 8 was at Cykesiopka's request. FWIW, the automated updates still didn't run on m-c today :\
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
Depends on: 1398829
Do we still want to do this?
Flags: needinfo?(cykesiopka.bmo)
(In reply to David Keeler [:keeler] (use needinfo) from comment #10)
> Do we still want to do this?

Yes, but sadly landing this will break automation until Bug 1398829 is fixed.
I'll try and take another stab at fixing that bug soon, but for now P3 I guess.
Flags: needinfo?(cykesiopka.bmo)
Priority: P1 → P3
Whiteboard: [psm-assigned] → [psm-blocked]
Oh, right, now I remember. No worries anyway - we were just doing a bit of triage on P1/psm-assigned bugs.
You need to log in before you can comment on or make changes to this bug.