Closed Bug 1618867 Opened 6 years ago Closed 5 years ago

Standardize /urlbar/tests names

Categories

(Firefox :: Address Bar, task, P3)

Product:
Firefox
Component:
Address Bar
Type:
task
Points:
2

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 76
Iteration:
76.1 - Mar 9 - Mar 22
Tracking Flags:
Tracking Status
firefox76 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1618867 - Standardize /urlbar/tests names. r?adw!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1618867 - Standardize /urlbar/tests names. r=adw
24.00 KB, patch
Details | Diff | Splinter Review

Our test names in urlbar/tests aren't standardized. Many are browser_urlbar_testName.js, but there are also a bunch of browser_urlbarTestName.js, and browser_testName.js. There's also a few browser_otherComponentName_testName.js, like browser_switchTab_override.js. Bug 1616631 added a urlbar/tests/browser/tips folder and the test names in there are also not standardized.

It would be nice if all our tests followed the browser_componentName_testName.js convention, where componentName is urlbar in most cases.

We should avoid oversimplification (e.g. browser_testName.js) since that could lead to conflicts with tests in other folders now or in the future. This would likely lead to undefined behaviour with the mach feature that allows you to run a test without specifying the pathname, for example mach mochitest browser_urlbar_interventions.js.

I'd like to vote for not including "urlbar" in the names... :-) It's just noise since all these tests are in the urlbar directory. The point about the mach feature is good though, but I'd argue if two tests have the same name, mach should let you disambiguate somehow and if it doesn't then that's a deficiency in mach.

Priority: -- → P3

Deciding on a name format for the future would be nice.
I'm also in favor of non duplicating what is in the path, reason for which I preferred to not change test names in Bug 1616631 to contain tip because they are in a tips/ folder. We should probably remove the urlbar part from some tests.
If this brings to some renaming, we should just pay attention to update intermittent failure bugs to track both names.

It sounds like we should file a bug against mach to provide a choice when multiple tests with the same name exist (show options with full path and allow to pick one by number). In general it's really easy to just get the relative path of a test from text editors (also VS Code on Windows has an add-on to get unix style relative path), so I'm not particularly worried about that.

I'll move forward with dropping urlbar from the names. For example urlbar/tests/browser/browser_urlbar_suggestedIndex.js will become urlbar/tests/browser/browser_suggestedIndex.js; and urlbar/tests/browser/browser_urlbarAboutHomeLoading.js will become urlbar/tests/browser/browser_aboutHomeLoading.js. Tests that refer to another component or feature, such as browser_switchTab_override.js, will remain unchanged.

Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Iteration: --- → 75.2 - Feb 24 - Mar 8

Marco suggested I rename these open bugs, or else do not rename those tests.

I decided to not rename browser_urlbar_event_telemetry and browser_urlbar_selection, just to avoid making unwieldy or too many changes to bugs. For the test of the bugs linked, I'll rename the bug or else they aren't tests in components/urlbar/tests. I'll rename the bugs after this one passes review.

Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd6f62b46de2 Standardize /urlbar/tests names. r=adw

Depends on D65192

Attachment #9130872 - Attachment is obsolete: true

I have a new revision up that rolls up the changes from the hotfix.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=326e66d0338349e9059fea45fa9146a29b8c1971

Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2a5f4092b45b Standardize /urlbar/tests names. r=adw

Sorry about that. This got tangled up in dependencies with bug 1620274 and I didn't double-check what I was pushing when all the rebasing was through.

Here's a new try run with both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b26b98716d7347ef333ab0f0ec28ad60faa02b2b

Blocks: 1620274
Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5b82efb2a20f Standardize /urlbar/tests names. r=adw

Oh, this is weird: autoland shows just one .ini file changed, when this patch should touch 50+ files. Phabricator shows that I'm editing all 50+ files. I'll investigate...

Flags: needinfo?(htwyford)

Okay, something went wrong with the VC on that revision. Not entirely sure what. I downloaded the raw diff from the old Phabricator and I'm just going to start fresh with a new Phabricator revision.

Attachment #9130548 - Attachment is obsolete: true
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef16e75c0df3 Standardize /urlbar/tests names. r=adw

The same thing I noted in comment 15 happened again. I contacted the Conduit team and they're stumped. They recommended I just upload a diff and land with checkin-needed. Sorry about the noise on this bug!

Attachment #9131583 - Attachment is obsolete: true
Flags: needinfo?(htwyford)
Attachment #9132243 - Flags: checkin?(opoprus)
Pushed by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66950fa024e1 Standardize /urlbar/tests names. r=adw! CLOSED TREE
Attachment #9131583 - Attachment is obsolete: false
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5e32bdf73dc2 Backed out changeset 66950fa024e1 for causing build bustages in /builds/worker/checkouts/gecko/browser/components/urlbar/tests/browser/tips/browser.ini CLOSED TREE

The issue here is that I was renaming the file browser_URLBarSetURI.js, but another patch renamed it to browser_UrlbarInput_setURI.js beneath me. Lando didn't catch this; this is being worked on in bug 1621284.

I'm preparing a patch that won't touch this file.

Flags: needinfo?(htwyford)

There's a new revision on Phabricator that does not touch browser_URLBarSetURI.js or browser_UrlbarInput_setURI.js and applies cleanly to central. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc9fcf8facf7f20195c4c8c54444ba0fe23ff8f7.

Attachment #9132243 - Flags: checkin?(opoprus)
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5aeb2757b68f Standardize /urlbar/tests names. r=adw

https://hg.mozilla.org/mozilla-central/rev/5aeb2757b68f

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox76: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 76
Iteration: 75.2 - Feb 24 - Mar 8 → 76.1 - Mar 9 - Mar 22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: