Closed
Bug 1428847
Opened 6 years ago
Closed 6 years ago
Move is_hidden and is_visible into BrowserTestUtils
Categories
(Firefox :: General, enhancement, P5)
Tracking
()
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: johannh, Assigned: kanika16047)
References
Details
Attachments
(1 file, 4 obsolete files)
There's a sad amount of duplicate is_hidden and is_visible functions in our tests (https://searchfox.org/mozilla-central/search?q=(is_hidden%7Cis_visible)&case=false®exp=true&path=head.js). We should probably move it into BTU.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → kanika16047
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review235814 Code analysis found 4 defects in this patch: - 4 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/test/siteIdentity/head.js:132 (Diff revision 1) > const insecureConnectionIcon = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled"); > if (!insecureConnectionIcon) { > // HTTP request, there should be no MCB classes for the identity box and the non secure icon > // should always be visible regardless of MCB state. > ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page"); > ok(is_hidden(connectionIcon), "connection icon should be hidden"); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: browser/base/content/test/siteIdentity/head.js:136 (Diff revision 1) > ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page"); > ok(is_hidden(connectionIcon), "connection icon should be hidden"); > } else { > // HTTP request, there should be a broken padlock shown always. > ok(classList.contains("notSecure"), "notSecure on HTTP page"); > ok(!is_hidden(connectionIcon), "connection icon should be visible"); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: browser/base/content/test/siteIdentity/head.js:154 (Diff revision 1) > is(classList.contains("mixedDisplayContent"), passiveLoaded && !(activeLoaded || activeBlocked), > "identityBox has expected class for passiveLoaded && !(activeLoaded || activeBlocked)"); > is(classList.contains("mixedDisplayContentLoadedActiveBlocked"), passiveLoaded && activeBlocked, > "identityBox has expected class for passiveLoaded && activeBlocked"); > > ok(!is_hidden(connectionIcon), "connection icon should be visible"); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: browser/base/content/test/siteIdentity/head.js:246 (Diff revision 1) > let promiseViewShown = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "ViewShown"); > doc.getElementById("identity-popup-security-expander").click(); > await promiseViewShown; > is(Array.filter(doc.getElementById("identity-popup-securityView") > .querySelectorAll("[observes=identity-popup-mcb-learn-more]"), > element => !is_hidden(element)).length, 1, Error: 'is_hidden' is not defined. [eslint: no-undef]
Updated•6 years ago
|
Status: NEW → ASSIGNED
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review236116 Thank you for picking this up! I'm holding off on fully reviewing this patch for now. Please upload an updated patch with fixes after you run the mochitests locally. It's a good idea to always check that your changes to test files are not breaking the tests. Here's how you can do that: ./mach mochitest <path_to_file/path_to_folder> If you have made changes to a head.js file, it's usually a good idea to run all mochitests that are in the folder containing your head.js to check if all the tests are passing. An head.js file is a support file that helps run other tests in a folder. ::: browser/base/content/test/permissions/head.js (Diff revision 1) > if (element.parentNode != element.ownerDocument) > return is_hidden(element.parentNode); > > return false; > } > - Please add the extra line you removed from here ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:228 (Diff revision 1) > + if (style.display == "-moz-popup") > + return ["hiding", "closed"].includes(element.state); > + > + // Hiding a parent element will hide all its children > + if (element.parentNode != element.ownerDocument) > + return is_hidden(element.parentNode); This should be BrowserTestUtils.is_hidden ... ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:244 (Diff revision 1) > + if (style.display == "-moz-popup" && element.state != "open") > + return false; > + > + // Hiding a parent element will hide all its children > + if (element.parentNode != element.ownerDocument) > + return is_visible(element.parentNode); See above. This should be BrowserTestUtils.is_visible
Attachment #8961443 -
Flags: review?(prathikshaprasadsuman) → review-
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review236354 Code analysis found 4 defects in this patch: - 4 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/base/content/test/siteIdentity/head.js:132 (Diff revision 1) > const insecureConnectionIcon = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled"); > if (!insecureConnectionIcon) { > // HTTP request, there should be no MCB classes for the identity box and the non secure icon > // should always be visible regardless of MCB state. > ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page"); > ok(is_hidden(connectionIcon), "connection icon should be hidden"); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: browser/base/content/test/siteIdentity/head.js:136 (Diff revision 1) > ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page"); > ok(is_hidden(connectionIcon), "connection icon should be hidden"); > } else { > // HTTP request, there should be a broken padlock shown always. > ok(classList.contains("notSecure"), "notSecure on HTTP page"); > ok(!is_hidden(connectionIcon), "connection icon should be visible"); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: browser/base/content/test/siteIdentity/head.js:154 (Diff revision 1) > is(classList.contains("mixedDisplayContent"), passiveLoaded && !(activeLoaded || activeBlocked), > "identityBox has expected class for passiveLoaded && !(activeLoaded || activeBlocked)"); > is(classList.contains("mixedDisplayContentLoadedActiveBlocked"), passiveLoaded && activeBlocked, > "identityBox has expected class for passiveLoaded && activeBlocked"); > > ok(!is_hidden(connectionIcon), "connection icon should be visible"); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: browser/base/content/test/siteIdentity/head.js:246 (Diff revision 1) > let promiseViewShown = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "ViewShown"); > doc.getElementById("identity-popup-security-expander").click(); > await promiseViewShown; > is(Array.filter(doc.getElementById("identity-popup-securityView") > .querySelectorAll("[observes=identity-popup-mcb-learn-more]"), > element => !is_hidden(element)).length, 1, Error: 'is_hidden' is not defined. [eslint: no-undef]
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review236116 I see that many of the mochitests are failing but I cannot understand why. Can you please tell if I need to import BTU wherever I'm using BTU.is_visible/is_hidden? Although I don't see it imported in other files.
Comment 7•6 years ago
|
||
Hi Kanika, it looks like you created a new changeset for the changes instead of amending your existing changeset. Please upload your original patch after amending it with the suggested changes for review. Here's how you can do that: - Select your original changeset by running 'hg up <changeset_ID>'. You can get the changeset ID of your original patch by running 'hg wip'. - Make necessary changes to files based on my suggestions in Comment 3. - Run 'hg commit --amend -m"your_commit_message" ' to commit those changes. - Run 'hg push review' to submit that changeset. I'll review your patch once this is done and we can figure out why the tests are failing. Thanks!
Updated•6 years ago
|
Attachment #8962032 -
Flags: review?(prathikshaprasadsuman)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8962032 -
Attachment is obsolete: true
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review237050 Thank you! There's a few things left to do before we can land this bug. Please change the commit message to make it sound imperative. Something like: "Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. r?prathiksha" And please rebase this patch on top of central. ::: browser/base/content/test/general/browser_bug633691.js:9 (Diff revision 2) > > add_task(async function test() { > const URL = "data:text/html,<iframe width='700' height='700'></iframe>"; > await BrowserTestUtils.withNewTab({ gBrowser, url: URL }, async function(browser) { > await ContentTask.spawn(browser, > { is_element_hidden_: is_element_hidden.toSource(), This test needs a little bit of work before it can run correctly. So I think it's a good idea to leave it as it is for now and solve it in a follow up bug! I guess that also means not removing is_hidden from browser/base/content/test/general/head.js. Could you please add it back there? :) ::: browser/base/content/test/permissions/head.js:1 (Diff revision 2) > ChromeUtils.import("resource:///modules/SitePermissions.jsm", this); Please remove this file ::: browser/base/content/test/permissions/head.js:1 (Diff revision 2) > ChromeUtils.import("resource:///modules/SitePermissions.jsm", this); Also, remove head.js from the support-files section of the browser.ini file that's in the same folder. ::: browser/base/content/test/webextensions/head.js (Diff revision 2) > // generates references to the former and generic add-ons manager code > // generates referces to the latter. > return (icon == "chrome://browser/content/extension.svg" || > icon == "chrome://mozapps/skin/extensions/extensionGeneric.svg"); > } > - Please add a new line here ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:216 (Diff revision 2) > PROCESSSELECTOR_CONTRACTID, null); > } > } > return Promise.all(promises).then(() => tab); > }, > Add a JS-doc style comment here ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:232 (Diff revision 2) > + if (element.parentNode != element.ownerDocument) > + return BrowserTestUtils.is_hidden(element.parentNode); > + > + return false; > + }, > + Add a JS-doc style comment here also
Attachment #8961443 -
Flags: review?(prathikshaprasadsuman) → review-
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review238374 ::: browser/base/content/test/permissions/head.js:1 (Diff revision 2) > ChromeUtils.import("resource:///modules/SitePermissions.jsm", this); ok, no, looks like this file still has an import. Please don't remove this file. There's also no need to change the support section of head.js in that case. Sorry :)
Comment 11•6 years ago
|
||
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. I'm also tagging Johann for a final review so we can be sure here.
Attachment #8961443 -
Flags: review- → review?(jhofmann)
Reporter | ||
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review239148 I think Prathiksha has mentioned everything that's left to do here. We should defer fixing browser_bug633691.js to a follow-up bug and the functions in BTU still need JSDoc at the top (see the other functions in that file for reference). Thank you for working on this!
Attachment #8961443 -
Flags: review?(jhofmann)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review239148 Thank you. I have pushed in an updated patch. :) Please take a look for the final review. And it'd be great to file that bug and work on it too!
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review238374 > ok, no, looks like this file still has an import. Please don't remove this file. There's also no need to change the support section of head.js in that case. Sorry :) Thanks a lot for the review! :)
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review239642 Almost done, thanks! Some things to note: It's always a good idea to run eslint before submitting a patch for review. Here's how you can do that: ./mach eslint <path_to_file/path_to_folder> We should probably also do a try run to make sure that the patch does not break anything(https://wiki.mozilla.org/ReleaseEngineering/TryServer) and this looks like a great opportunity for you to trigger your first try run. To do that, you'll need to first get Level 1 access. You can read about how to get commit access here(https://www.mozilla.org/en-US/about/governance/policies/commit/). Once you have filed a bug for it, feel free to CC me or Johann to vouch for you. :) ::: browser/base/content/test/general/browser_bug633691.js:10 (Diff revision 3) > add_task(async function test() { > const URL = "data:text/html,<iframe width='700' height='700'></iframe>"; > await BrowserTestUtils.withNewTab({ gBrowser, url: URL }, async function(browser) { > await ContentTask.spawn(browser, > { is_element_hidden_: is_element_hidden.toSource(), > - is_hidden_: is_hidden.toSource() }, > + is_hidden_: BrowserTestUtils.is_hidden.toSource() }, Please revert this change ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:224 (Diff revision 3) > + * > + * @param {Element} element > + * The element which is to be checked. > + * > + * @return {boolean} > + * trailing spaces here, please remove this line :) ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:249 (Diff revision 3) > + * > + * @param {Element} element > + * The element which is to be checked. > + * > + * @return {boolean} > + * trailing spaces here also, remove the line
Attachment #8961443 -
Flags: review?(prathikshaprasadsuman) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review240408 Please run eslint before submitting a patch for review. You can avoid all the trailing spaces and other linting errors that way. Here's how you can do that: ./mach eslint <path_to_file/path_to_folder> I will r+ this patch once we have good try results since this patch includes a lot of changes to test files. You can schedule a try run once you get your commit access on reviewboard under the automation menu. Feel free to ask questions if you get stuck. ::: browser/base/content/test/permissions/head.js (Diff revision 5) > ChromeUtils.import("resource:///modules/SitePermissions.jsm", this); > - I think you need a newline at the end of this file ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:223 (Diff revision 5) > + * Checks if a DOM element is hidden. > + * > + * @param {Element} element > + * The element which is to be checked. > + * > + * @return {boolean} trailing whitespaces here ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:247 (Diff revision 5) > + * Checks if a DOM element is visible. > + * > + * @param {Element} element > + * The element which is to be checked. > + * > + * @return {boolean} trailing whitespaces here also
Attachment #8961443 -
Flags: review?(prathikshaprasadsuman) → review-
Comment hidden (mozreview-request) |
Comment 21•6 years ago
|
||
Hi Kanika, I just saw your try results and it looks like mochitests weren't included in your last try run. Please schedule a new try run with this as your try syntax: try: -b do -p linux64,macosx64,win64 -u mochitests -t none --artifact
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review242476 Thank you for working on this. Just a few things left to change. Please rebase your patch on central to avoid toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js from failing on try. :) ::: browser/base/content/test/general/head.js:477 (Diff revision 6) > - ok(is_visible(element), msg || "Element should be visible"); > + ok(BrowserTestUtils.is_visible(element), msg || "Element should be visible"); > } > > function is_element_hidden(element, msg) { > isnot(element, null, "Element should not be null, when checking visibility"); > - ok(is_hidden(element), msg || "Element should be hidden"); > + ok(BrowserTestUtils.is_hidden(element), msg || "Element should be hidden"); Please revert this change to stop browser/base/content/test/general/browser_bug633691.js from failing :) ::: toolkit/mozapps/extensions/test/browser/head.js:476 (Diff revision 6) > - if (aElement.parentNode != aElement.ownerDocument) > - return is_hidden(aElement.parentNode); > - > - return false; > -} > And remove the extra line from here
Attachment #8961443 -
Flags: review?(prathikshaprasadsuman) → review-
Comment hidden (mozreview-request) |
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review242908 Looks great, thank you!
Attachment #8961443 -
Flags: review?(prathikshaprasadsuman) → review+
Assignee | ||
Comment 25•6 years ago
|
||
Thank you :) Is there a need for setting the check-in needed flag?
Comment 26•6 years ago
|
||
(In reply to kanika16047 from comment #25) > Thank you :) > Is there a need for setting the check-in needed flag? To get your patch landed? Yes. :) I scheduled another try run for you today and it looks like toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js still needs a fix, and sorry, I wasn't very clear in my review about what needs to get fixed there. Could you look into it before setting the checkin-needed flag? I'm very busy this week. You can ping Johann on IRC if you have any questions.
Reporter | ||
Comment 27•6 years ago
|
||
Hi Kanika, are you still working on this? I'd be happy to help you push this over the finish line :)
Flags: needinfo?(kanika16047)
Assignee | ||
Comment 28•6 years ago
|
||
Hi, I'm really sorry I got busy with the exams. Yes, I'm continuing working on this. Thank you for understanding.
Flags: needinfo?(kanika16047)
Reporter | ||
Comment 29•6 years ago
|
||
Ok, sure, no worries!
Assignee | ||
Comment 30•6 years ago
|
||
Hi, I ran ./mach mochitest browser/base/content/test/general/browser_bug633691.js after rebasing with the latest code and the test ran successfully. Should I push the patch for review to be able to trigger another try run?
Comment 31•6 years ago
|
||
(In reply to kanika16047 from comment #30) > Hi, I ran ./mach mochitest > browser/base/content/test/general/browser_bug633691.js after rebasing with > the latest code and the test ran successfully. Should I push the patch for > review to be able to trigger another try run? Yes, you can upload the updated patch(run hg push review again) and schedule another try run on it. If the try results look green, we can then land your latest revision. :)
Comment hidden (mozreview-request) |
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8961443 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/230190/#review248334 Code analysis found 5 defects in this patch: - 5 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/mozapps/extensions/test/browser/browser_list.js:169 (Diff revision 8) > is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden"); > > info("Addon 5"); > addon = items["Test add-on 5"]; > addon.parentNode.ensureElementIsVisible(addon); > await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link"))); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: toolkit/mozapps/extensions/test/browser/browser_list.js:225 (Diff revision 8) > is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden"); > > info("Addon 8"); > addon = items["Test add-on 8"]; > addon.parentNode.ensureElementIsVisible(addon); > await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link"))); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: toolkit/mozapps/extensions/test/browser/browser_list.js:247 (Diff revision 8) > is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden"); > > info("Addon 9"); > addon = items["Test add-on 9"]; > addon.parentNode.ensureElementIsVisible(addon); > await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link"))); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: toolkit/mozapps/extensions/test/browser/browser_list.js:524 (Diff revision 8) > is(Object.keys(items).length, EXPECTED_ADDONS, "Should be the right number of add-ons installed"); > > info("Addon 10"); > let addon = items["Test add-on 10"]; > addon.parentNode.ensureElementIsVisible(addon); > await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link"))); Error: 'is_hidden' is not defined. [eslint: no-undef] ::: toolkit/mozapps/extensions/test/browser/browser_list.js:545 (Diff revision 8) > is(get_node(addon, "error-link").href, infoURL, "Error link should be correct"); > > info("Addon 11"); > addon = items["Test add-on 11"]; > addon.parentNode.ensureElementIsVisible(addon); > await TestUtils.waitForCondition(() => !is_hidden(get_node(addon, "error-link"))); Error: 'is_hidden' is not defined. [eslint: no-undef]
Reporter | ||
Comment 34•6 years ago
|
||
It seems like you'll have to replace the usage in toolkit/mozapps/extensions/test/browser/browser_list.js as well :)
Comment hidden (mozreview-request) |
Comment 36•6 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #0) > We should probably move it into BTU. Since this is also useful for mochitest-plain and mochitest-chrome it would probably be better in TestUtils, or even better, part of SimpleTest.js which is already in most of those test scopes (though I don't understand how much we're allowed to change that).
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #34) > It seems like you'll have to replace the usage in > toolkit/mozapps/extensions/test/browser/browser_list.js as well :) Hi, fixed that but there are a few more test jobs that are failing on try run. Can you please help me figure out what else needs to be changed?
Reporter | ||
Comment 38•6 years ago
|
||
(In reply to kanika16047 from comment #37) > (In reply to Johann Hofmann [:johannh] from comment #34) > > It seems like you'll have to replace the usage in > > toolkit/mozapps/extensions/test/browser/browser_list.js as well :) > > Hi, fixed that but there are a few more test jobs that are failing on try > run. > Can you please help me figure out what else needs to be changed? I think you just forgot to replace these occurrences: https://searchfox.org/mozilla-central/search?q=is_hidden&case=false®exp=false&path=browser_details.js https://searchfox.org/mozilla-central/search?q=is_hidden&case=false®exp=false&path=browser_discovery.js That should be simple to fix :) Thanks for all the work!
Reporter | ||
Comment 39•6 years ago
|
||
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #36) > (In reply to Johann Hofmann [:johannh] from comment #0) > > We should probably move it into BTU. > > Since this is also useful for mochitest-plain and mochitest-chrome it would > probably be better in TestUtils, or even better, part of SimpleTest.js which > is already in most of those test scopes (though I don't understand how much > we're allowed to change that). Hm, yeah, I agree, but considering that this is a pretty big mentored bug I'd prefer to push that into a follow-up (where we could also figure out the SimpleTest parts). I realize that that would mean touching all that code again, but I'm honestly just happy with the state of having one common implementation somewhere, in thise case BTU. What do you think?
Comment 40•6 years ago
|
||
Yeah, that's reasonable.
Reporter | ||
Comment 41•6 years ago
|
||
Hi Kanika, would you have time to fix up the last two occurrences mentioned in comment 38? It would be great to be able to land this.
Flags: needinfo?(kanika16047)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8961443 -
Attachment is obsolete: true
Reporter | ||
Comment 44•6 years ago
|
||
mozreview-review |
Comment on attachment 8982213 [details] Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils https://reviewboard.mozilla.org/r/248192/#review254460 This looks good but something is not right with the way these commits are organized, can you please squash them into a single commit? Thanks :)
Attachment #8982213 -
Flags: review?(jhofmann)
Reporter | ||
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8982212 [details] Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils https://reviewboard.mozilla.org/r/248190/#review254462
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8982213 -
Attachment is obsolete: true
Assignee | ||
Comment 47•6 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #44) > Comment on attachment 8982213 [details] > Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils > > https://reviewboard.mozilla.org/r/248192/#review254460 > > This looks good but something is not right with the way these commits are > organized, can you please squash them into a single commit? > > Thanks :) Sorry for the delay. Does this look better now? I followed http://docs.firefox-dev.tools/contributing/making-prs.html to squash commits.
Flags: needinfo?(kanika16047)
Reporter | ||
Comment 48•6 years ago
|
||
mozreview-review |
Comment on attachment 8982212 [details] Bug 1428847 - Moved is_hidden and is_visible into BrowserTestUtils https://reviewboard.mozilla.org/r/248190/#review256974 Unfortunately it's not really what we need (the entire patch not a subset of it). Maybe you can ping me on IRC or Slack and we can quickly resolve this together? :) Thanks!
Attachment #8982212 -
Flags: review?(jhofmann)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8982212 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Reporter | ||
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8985142 [details] Bug 1428847 - Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. https://reviewboard.mozilla.org/r/250832/#review257210 Looks great, thank you!
Attachment #8985142 -
Flags: review?(jhofmann) → review+
Comment 52•6 years ago
|
||
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/684094d78fb4 Move is_hidden and is_visible helper functions into BrowserTestUtils.jsm. r=johannh
Comment 53•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/684094d78fb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in
before you can comment on or make changes to this bug.
Description
•