Introduce common JSM for security/manager/tools/ scripts

REOPENED
Assigned to

Status

()

enhancement
P3
normal
REOPENED
2 years ago
a year ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

(Depends on 1 bug, {stale-bug})

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 unaffected)

Details

(Whiteboard: [psm-blocked])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
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+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/121e4d470c11
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 8

2 years ago
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 → ---
(Assignee)

Updated

2 years ago
Depends on: 1398829
Do we still want to do this?
Flags: needinfo?(cykesiopka.bmo)
(Assignee)

Comment 11

a year ago
(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.