Closed Bug 1594082 Opened 2 months ago Closed 2 months ago

Remove test code related to XUL about:addons

Categories

(Toolkit :: Add-ons Manager, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: mstriemer, Assigned: stensonj, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

getAnonymousElementByAttribute is going away and we've got test code still using it. This is dead test code from the XUL about:addons that can be removed and isn't currently running.

The browser_interaction_telemetry.js file is using it, but we should take a glance at some other files to make sure they don't have similar dead code.

Mentor: mstriemer
Blocks: 1591145

Hi there, I would like to take on this bug. From what I gather from your description, to solve this bug I would need to remove the lines of code containing the "getAnonymousElementByAttribute" code? I have searched for "getAnonymousElementByAttribute" on searchfox.org and found occurrences of the code in the file you mentioned. So delete the lines of code in that file?

Hi there, just a quick re-comment as I did not request information from the report below my comment. Sorry I am new to bugzilla and debugging mozilla code as I am doing it for an Open Source Development module at my University. I may also need some guidance while completing this bug if that is okay.

Flags: needinfo?(mstriemer)

Hi Jacob, great to hear you're interested in looking at this, thanks!

For some context, the test file I mentioned used to be run on two versions of the same page. Most of the tests and helpers will have an if to determine how to proceed on the active version. The check was generally something like doc.ownerGlobal != gManagerWindow [1] and that's the case we want to keep.

For the example I linked that function would become:

async function enableAndDisable(doc, row) {
  let toggleButton = row.querySelector('[action="toggle-disabled"]');
  let disabled = BrowserTestUtils.waitForEvent(row, "update");
  toggleButton.click();
  await disabled;
  let enabled = BrowserTestUtils.waitForEvent(row, "update");
  toggleButton.click();
  await enabled;
}

Let me know if there's any other way I can help!

[1] https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/toolkit/mozapps/extensions/test/browser/browser_interaction_telemetry.js#91

Flags: needinfo?(mstriemer)

Hi, thanks for your reply. Just to clarify before I start work on this, I would need to remove everything inside the if else statement on line 91 of the browser_interaction_telemetry.js file EXCEPT for the code that would be executed if the IF condition was to be true?
Could the same be said for the if statement on line 118? Remove all code EXCEPT for the code that would be executed if the IF condition were true?
If I believe the above to be true. I would need to do the same on lines 147 and 193? Thanks

It looks like all of these [1] helpers have two implementations. They should each be reduced down to the one case, and that sounds right for what you're suggesting.

[1] https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/toolkit/mozapps/extensions/test/browser/browser_interaction_telemetry.js#74-214

Awesome. Should I still change the code from line 163 to 190 as well? I only ask as there is no call to "getAnonymousElementByAttribute" code in that function? Just to ensure I am doing this correctly.

Hi again. Could you please clarify this for me before I start work on it?

Flags: needinfo?(mstriemer)

Yeah, that's still dead code that needs to be removed. Thanks

Flags: needinfo?(mstriemer)

Hi again. I seem to have lost the previous commit for this bug on my virtual machine and and cannot rebase my changes with it. Therefore I will need to send a new commit in a couple minutes time. Apologies for this, I hope this does not mess anything up.

Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89a84b56be08
Remove test code related to XUL about:addons, r=mstriemer
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

(In reply to Jacob Stenson from comment #10)

Hi again. I seem to have lost the previous commit for this bug on my virtual machine and and cannot rebase my changes with it. Therefore I will need to send a new commit in a couple minutes time. Apologies for this, I hope this does not mess anything up.

No worries about submitting a new revision. In the future if you want to update a revision you can include the URL to the revision you want to update. So in this case your commit message would've been something like:

Bug 1594082 - Remove test code related to XUL about:addons, r?mstriemer

Differential Revision: https://phabricator.services.mozilla.com/D54203

If you check your commit after submitting a commit with hg log -r . -v to see the full commit message, your last commit should have something similar.

Assignee: nobody → stensonj
Flags: qe-verify-

(In reply to Mark Striemer [:mstriemer] from comment #14)

(In reply to Jacob Stenson from comment #10)

Hi again. I seem to have lost the previous commit for this bug on my virtual machine and and cannot rebase my changes with it. Therefore I will need to send a new commit in a couple minutes time. Apologies for this, I hope this does not mess anything up.

No worries about submitting a new revision. In the future if you want to update a revision you can include the URL to the revision you want to update. So in this case your commit message would've been something like:

Bug 1594082 - Remove test code related to XUL about:addons, r?mstriemer

Differential Revision: https://phabricator.services.mozilla.com/D54203

If you check your commit after submitting a commit with hg log -r . -v to see the full commit message, your last commit should have something similar.

Ah okay, thank you. I appreciate the tips, I am still learning about the whole committing process so tips and help like this is brilliant! Thanks again :)

You need to log in before you can comment on or make changes to this bug.