testrail link: https://testrail.stage.mozaws.net/index.php?/cases/view/76202 Steps: 1. Click on the checkbox in front of Search Expected results: Checkbox is unchecked. Search bar is removed from A-S page.

I'd like to work on this. Should I create a new test file for "new tabs preferences" in chrome://browser/extensions/activity-stream/test/functional/mochitest or somewhere else?

Given that the preferences sidebar is moving to about:preferences, the test should just cover whether the preference triggers updates to the content. Disable and re-enable of top sites is tested https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js#14-29 You can probably add the pushPref for showing/hiding search to this file and fix bug 1410431 at the same time similar to how the above linked code tests both.

I've added 2 tests. While the one for enabling the search bar passes, I can't figure out why the other one fails. Can you please point out my mistakes? Its my first time as a contributor here. :)

Did you try setting the prefs like the tests above the ones you added, e.g., `await pushPrefs(["browser.newtabpage.activity-stream.showTopSites", false]);` (but for the appropriate search pref)? As noted in comment 2, the sidebar preferences will be going away, so let's avoid relying on that UI for the test.

Thanks for the help. I did that and all tests now pass. Although there seems to be another test for a search bar but it seems like a placeholder one. This: test_newtab(function test_render_search() { let search = content.document.getElementById("newtab-search-text"); ok(search, "Got the search box"); isnot(search.placeholder, "search_web_placeholder", "Search box is localized"); }); Should I delete this? The newtab page has no element of id newtab-search-text

There is indeed a #newtab-search-text as a child of the .search-wrapper that you're checking. Maybe the test should just be renamed `test_render_localized` as it is checking for something that isn't covered by your tests.

Can anybody review this?

Mardak, can you review the patches here?

Comment on attachment 8953823 [details] patches for #1410425 and #1410431 - make changes after review https://reviewboard.mozilla.org/r/223006/#review231286 There's various nits to clean up. Also, the patch should be a single commit instead of spread across two as we don't need the first intermediate commit. ::: browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js:3 (Diff revision 1) > "use strict"; > > -test_newtab(function test_render_search() { > +test_newtab(function test_render_localised() { nit: let's keep the spelling consistent with the one in the function: "localized" ::: browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js:10 (Diff revision 1) > ok(search, "Got the search box"); > isnot(search.placeholder, "search_web_placeholder", "Search box is localized"); > }); > > +// check if the searchbar is enabled > +test_newtab(function test_render_searchbar() { This is more related to the other 2 added tests further down, so let's move this after the topsites tests just above the other searchbar tests. ::: browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js:12 (Diff revision 1) > }); > > +// check if the searchbar is enabled > +test_newtab(function test_render_searchbar() { > + let searchWrapper = content.document.querySelector(".search-wrapper"); > + nit: there's unnecessary whitespace here ::: browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js:13 (Diff revision 1) > > +// check if the searchbar is enabled > +test_newtab(function test_render_searchbar() { > + let searchWrapper = content.document.querySelector(".search-wrapper"); > + > + // assuming the checkbox is already checked This comment referring to "checkbox" is somewhat confusing. Maybe just remove this comment and have the one before the function also mention "enabled by default" ::: browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js:45 (Diff revision 1) > -// then checks if the searchbar is disabled > -test_newtab(function test_searchbar_enabled() { > - let searchbarToggle = content.document.querySelector("#showSearch"); > - > + async before({pushPrefs}) { > + await pushPrefs(["browser.newtabpage.activity-stream.showSearch", false]); > + }, > + test: function test_render_no_searchbar() { > - let searchWrapper = content.document.querySelector(".search-wrapper"); > + let searchWrapper = content.document.querySelector(".search-wrapper"); > - > + ok(!searchWrapper, "No Search bar") These set of 3 tests seem like a pattern that we might want for other section tests, so it might be useful to think about a higher order function. Not something we need to fix here unless you want to play around with that. One note is that could get tricky is that this is a mix of chrome and content functions. There is a way to add content helpers now: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/extensions/activity-stream/test/functional/mochitest/head.js#69-75 And there are ways to chain data from before to test to after: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/extensions/activity-stream/test/functional/mochitest/head.js#151-153 But again, not needed for this fix.

Thanks for reviewing, I will fix that. So when I make these changes and I push them it will create another commit, so should I just delete the commits before and make a new one? I'm just used to making PRs using Github (sorry)

Are you using mercurial or git? Both provide ways to --amend the previous commit as well as other ways to edit / rebase / squash existing commits.

please review