Closed Bug 1590358 Opened 3 months ago Closed 2 months ago

Replace static assertions in browser chrome tests with info()

Categories

(Remote Protocol :: Agent, task, P3)

Tracking

(firefox72 fixed)

RESOLVED FIXED
Firefox 72
Tracking Status
firefox72 --- fixed

People

(Reporter: whimboo, Assigned: walshd9, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

In the browser-chrome tests for the remote protocol we have a lot of static assertions like the following:

ok(true, "'Page.loadEventFired' fired");

Those actually don't test anything but are simply used for additional logging output. As such we should convert them all to info("'Page.loadEventFired' fired").

All the tests can be found here:

https://searchfox.org/mozilla-central/source/remote/test/browser/

It might make sense to wait with this bug until bug 1589625 has been fixed.

Mentor: hskupin, mjzffr
Whiteboard: [lang=js]

Hi, I'm a first time contributor, can I have a crack at this bug?
From what I read above, it simply requires changing the static assertions to info()

Flags: needinfo?(hskupin)

Hi Dan. It's great to have you here, and to see that you have interest to work on this bug. So yes, go ahead. It's yours. And your description sounds all right. I will assign the bug to you once the first version of the patch got uploaded. Thanks!

Flags: needinfo?(hskupin)

Ok, cheers. Will start on it now

Uploaded my commit. Was this what was needed? Anything else need to be done? Cheers

Flags: needinfo?(hskupin)

That's totally fine. I will have a look at it soon. Thanks a lot for the patch.

Flags: needinfo?(hskupin)

I've done the fix again on the updated version. It's in a second commit, wasn't sure how to submit a revision of a previous commit.

When you make follow-up changes you have to amend the changes to the former commit (hg commit --amend). Otherwise you have to edit the history of your bookmark (hg histedit). See https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#changing-code-after-reviews.

I assume the first commit is not necessary anymore? If that is the case can you please abandon the revision in Phabricator? You can find that in the dropdown at the very end of the page. Once that has been done I will review your patch.

Assignee: nobody → walshd9
Status: NEW → ASSIGNED

Hi, do you want me to abandon the most up to date commit and then try to amend to the old commit, or just abandon the old commit? Cheers

Flags: needinfo?(hskupin)

First please update your commit by using Mercurial (hg) by following what I mentioned above. Then push those changes to Phabricator. All of them should end-up in the first revision, and the second one could then be closed.

Flags: needinfo?(hskupin)

Hi, I've tried to make the changes to D53021 using histedit but can only get changes to effect the commit that is on D53352. As D53352 is the commit with the up-to-date code in it, shall I mark D53021 as abandoned so that D53352 can be used?

Flags: needinfo?(hskupin)

How many local changesets do you still have after histedit? When you check the commit message details, which phabricator revision is referenced? If there is only D53352, then this would explain it, and you want to close the first review. For a clear answer I would have to know which commands you actually performed after the hg pull and hg up central.

This time it might be good to just abandon this revision which doesn't contain the right code. If it's the first, and your local commit message references the second, then just do it. Also feel free to join us on IRC to get quicker help.

Also FYI I'm reading my bugmail so it's not necessary to set needinfo for each comment. Thanks.

Flags: needinfo?(hskupin)
Attachment #9108708 - Attachment is obsolete: true

Hi, I've now abandoned the first commit and left the second, up-to-date, commit active, it'll just need review.

I had tried using "hg wip" and "hg log -l" to try to get the commit code so I would then have been able to do "hg up <commit code>". But even after talking with the people on IRC, I still couldn't see the first commit. Also, thanks for letting me know about the needinfo.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f9ddcc4d7d0
Replace static assertions in browser chrome tests with info(), r=remote-protocol-reviewers,whimboo

The problem here is that the test remote/test/browser/input/browser_dispatchMouseEvent.js doesn't contain any assertions now. We should assert at least one single item. Maybe we should leave the last line as is:

ok(true, "All events detected");

Has this already been done or would you like me to do this?

Flags: needinfo?(walshd9)

No, it hasn't and is waiting for you to get this done. Please make sure to amend those changes in your current patch, so that it keeps in the same revision on Phabricator. Thanks!

I've amended the commit. Is there a way to edit the summary, as I added some characters by accident? Cheers

You can do locally via hg histedit or in Phabricator directly by using the Edit Revision link at the top right.

Attachment #9109301 - Attachment description: Bug 1590358 - Replace static assertions in browser chrome tests with info(), r=whimboo → Bug 1590358 - [remote] Replace static assertions in browser chrome tests with info(), r=whimboo
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fd9cbf624f8
[remote] Replace static assertions in browser chrome tests with info(), r=remote-protocol-reviewers,whimboo
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
You need to log in before you can comment on or make changes to this bug.