Closed
Bug 1388823
Opened 7 years ago
Closed 7 years ago
Sync shield-recipe-client v65 from GitHub (commit ff1b680c)
Categories
(Firefox :: Normandy Client, defect)
Firefox
Normandy Client
Tracking
()
RESOLVED
FIXED
People
(Reporter: mythmon, Assigned: mythmon)
References
Details
Attachments
(1 file, 1 obsolete file)
The major change to the add-on since the last sync (Bug 1384652, v61) is opt-out studies support.
Assignee | ||
Updated•7 years ago
|
Summary: Sync shield-recipe-client v64 from GitHub (commit c3bfdcb9) → Sync shield-recipe-client v65 from GitHub (commit ff1b680c)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8895921 [details] Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c) https://reviewboard.mozilla.org/r/167202/#review172786 There are some errors on try to do with the all_files_referenced.js checks. I think it (for whatever reason) isn't realizing you *are* referencing the content / frame scripts. I don't know why. I suggest you add them to the whitelist to get unblocked for now. There's also an unused whitelist entry warning for IndexedDB.jsm that I think you can then just remove, maybe? I thought we removed the dependency on ajv again? I don't see it being used apart from using Cu.unload. I'm also confused because this PR seems to include about:studies support but that isn't mentioned either here or in bug 1388823 . Is that just part of opt-out studies support? Finally, your trypush only ran mochitest-e10s-browser-1, not the other however many. Was that intentional? It rather seems like the others should be running too...
Attachment #8895921 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to :Gijs from comment #2) > Comment on attachment 8895921 [details] > Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c) > > https://reviewboard.mozilla.org/r/167202/#review172786 > > There are some errors on try to do with the all_files_referenced.js checks. > I think it (for whatever reason) isn't realizing you *are* referencing the > content / frame scripts. I don't know why. I suggest you add them to the > whitelist to get unblocked for now. There's also an unused whitelist entry > warning for IndexedDB.jsm that I think you can then just remove, maybe? Thanks for debugging this. I'll adjust the exceptions. > I thought we removed the dependency on ajv again? I don't see it being used > apart from using Cu.unload. Nice catch. I think this may have gotten included because I forgot to cleanup a generated files directory. I'll make sure it stays gone. > I'm also confused because this PR seems to include about:studies support but > that isn't mentioned either here or in bug 1388823 . Is that just part of > opt-out studies support? Yes, about:studies is a part of opt-out studies. It is the UI that allows users to opt out of out-out studies. >> Finally, your trypush only ran mochitest-e10s-browser-1, not the other > however many. Was that intentional? It rather seems like the others should > be running too... Oops. I copied another try line without reading it too closely. I'll make sure to run a better set of tests this time.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8895921 [details] Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c) https://reviewboard.mozilla.org/r/167202/#review172944 r=me assuming try comes back green
Attachment #8895921 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Try did not come back green, there were a couple Windows-specific test failures. Those are fixed now. Can you re-r? this?
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/87a67b857362 Sync shield-recipe-client v65 from GitHub (commit ff1b680c) r=Gijs
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8895921 [details] Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c) Approval Request Comment [Feature/Bug causing the regression]: Shield Opt-out studies [User impact if declined]: Tab Center studies will not be able to run. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: Manual testing is being performed, bugs will be filed blocking bug 1388092. Test plan is at https://wiki.mozilla.org/QA/Shield_Opt_Out_Studies . I don't know if this needs to block the uplift. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Unlikely [Why is the change risky/not risky?]: This adds new capabilities to Shield, that are not used except for a small percentage of users in experimental groups. Of course, there may be bugs. [String changes made/needed]: None
Attachment #8895921 -
Flags: approval-mozilla-beta?
I had to back this out for frequent failures in various browser-chrome tests: https://treeherder.mozilla.org/logviewer.html#?job_id=123128661&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=123118446&repo=autoland https://hg.mozilla.org/integration/autoland/rev/9084b3bb0e78efae2701e10b9187cdba14bbc565
Flags: needinfo?(mcooper)
Comment 12•7 years ago
|
||
Tracking to make sure we follow up on landing and uplifting.
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Gijs: I've put up a potential fix for the test errors; the fix is in AboutPages.jsm in the sendStudyList function (I couldn't figure out how to link to the lines but you can see the diff on Github: https://github.com/mozilla/normandy/compare/master...Osmose:child-process-fail). It's otherwise identical to the backed-out patch. My thinking is that the test failures close about:studies after it fires off a request for studies to the parent process, but before the parent process sends the results back, and it's the attempt to send a message to a non-existent target that is failing during the tests. I did a try run to test the fix and bc1 and bc5 were fixed (https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddefa1dca3632540412027c2ef340a407772edd7), although other chunks failed due to my test missing fixes from this patch. I've triggered a try run for the updated patch on the test suites in question to see if it works.
Flags: needinfo?(mcooper)
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8898034 [details] Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c) https://reviewboard.mozilla.org/r/169338/#review174692 rs=me on the changes in https://github.com/mozilla/normandy/compare/master...Osmose:child-process-fail . I guess this is good to go if the tests are green, we can fix the IO stuff in a followup if necessary.
Attachment #8898034 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 16•7 years ago
|
||
Pushed by mkelly@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e76b1b892fc8 Sync shield-recipe-client v65 from GitHub (commit ff1b680c) r=Gijs
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e76b1b892fc8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 18•7 years ago
|
||
Comment on attachment 8895921 [details] Bug 1388823 - Sync shield-recipe-client v65 from GitHub (commit ff1b680c) Looks like we have worked out most of the issues, with one known perf test failure that may be from the test methodology. Let's try this in 56 beta 5, so we can run studies in beta 56.
Attachment #8895921 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/eee5b1dfb49f
Assignee | ||
Updated•7 years ago
|
Attachment #8895921 -
Attachment is obsolete: true
Updated•6 years ago
|
Product: Shield → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•