1428108 - set browser.newtabpage.activity-stream.debug by default in local builds

Status: RESOLVED FIXED

(Firefox :: Messaging System, enhancement, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

One highly likely reason that bug 1428106 was not noticed sooner is because we don't have warnings turned on by default in local builds.  We should do that.  :-)

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3924

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review commit: https://reviewboard.mozilla.org/r/219956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219956/

Comment 3

[Ed Lee :Mardak]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review225778

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/301951ce868f
Set browser.newtabpage.activity-stream.debug by default in local builds. r=Mardak

Comment 5

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 301951ce868f (bug 1428108) for xpcshell failures at browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=301951ce868f1df6d05699e03ab753559d7b31d1&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162232855&repo=autoland&lineNumber=2633

Backout: https://hg.mozilla.org/integration/autoland/rev/e0ad971db3e7e73e35a6cab94652b31a1936c403

Comment 6

[Andrei Oprea [:andreio]]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219956/diff/1-2/

Comment 7

[Andrei Oprea [:andreio]]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219956/diff/2-3/

Comment 8

[Andrei Oprea [:andreio]]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219956/diff/3-4/

Comment 9

[Andrei Oprea [:andreio]]

And try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2e868e7cbcf9f3a0a6a566712606746df1a09bc

Comment 10

[Ed Lee :Mardak]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review226586

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:88
(Diff revision 4)
>  
>  add_task(async function test_override_activity_stream_enabled() {
>    let notificationPromise = await setupASPrerendered();
>  
> +  if (!IS_RELEASE_OR_BETA && !MOZILLA_OFFICIAL) { // Check if local build
> +    Assert.equal(aboutNewTabService.defaultURL, ACTIVITY_STREAM_PRERENDER_DEBUG_URL, "Newtab URL should be the debug activity stream prerendered URL");

Instead of doing this check everywhere, I think it would be better that pretty much all tests run assuming debug is false. One exception is for a new test to check if the false value is the default for MOZILLA_OFFICIAL or not.

Comment 11

[Andrei Oprea [:andreio]]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219956/diff/4-5/

Comment 12

[Andrei Oprea [:andreio]]

Try run for the updated patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=432ca512096d721af638cc62ecc20db390a3a087&selectedJob=163104969

Comment 13

[Ed Lee :Mardak]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review228148

We have less coverage with this change. My suggestion was to make most tests run with pref debug = false even for non-OFFICIAL builds. That way the existing assertions can be left untouched. For the newly added test, it can check if the default value of the debug pref is true for non-OFFICIAL.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js
(Diff revision 5)
>  });
>  
>  add_task(async function test_override_activity_stream_enabled() {
>    let notificationPromise = await setupASPrerendered();
>  
> -  Assert.equal(aboutNewTabService.defaultURL, ACTIVITY_STREAM_PRERENDER_URL, "Newtab URL should be the default activity stream prerendered URL");

Now there are no tests checking that `.defaultURL` is `ACTIVITY_STREAM_PRERENDER_URL`.

Comment 14

[Andrei Oprea [:andreio]]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219956/diff/5-6/

Comment 15

[Andrei Oprea [:andreio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=040e741468e3bd45663f4b8aebe0ec4a088309de&selectedJob=163642244

Comment 16

[Ed Lee :Mardak]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review228354

You should also update/comment the !issues in mozreview

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:79
(Diff revision 6)
>  });
>  
>  add_task(async function test_override_activity_stream_enabled() {
>    let notificationPromise = await setupASPrerendered();
> +  // Don't run in debug mode regardless of build type
> +  Services.prefs.setBoolPref(ACTIVITY_STREAM_DEBUG_PREF, false);

All of these come immediately after a `setupASPrerendered()`, and setting `DEBUG_PREF` to `false` seems reasonable to do there as it does control which prerendered endpoint to use.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:142
(Diff revision 6)
>    Assert.equal(aboutNewTabService.activityStreamLocale, "en-US",
>      "The locale for testing should be en-US");
>  });
>  
> +add_task(async function test_debug_mode() {
> +  await setupASPrerendered();

Does this need to be called for testing debug mode?

Comment 17

[Andrei Oprea [:andreio]]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219956/diff/6-7/

Comment 18

[Ed Lee :Mardak]

Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review228412

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7d8e299023a
Set browser.newtabpage.activity-stream.debug by default in local builds. r=Mardak

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e7d8e299023a