1676736 - Use Assert.rejects to test exceptions in Remote browser chrome tests

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P3)

Description

[Julian Descottes [:jdescottes]]

From https://phabricator.services.mozilla.com/D96705#inline-545263

We could rewrite exception asserts using Assert.rejects.
Example:

    let errorThrown = "";
    try {
      await Emulation.setTouchEmulationEnabled({ enabled });
    } catch (e) {
      errorThrown = e.message;
    }
    ok(
      errorThrown.match(/enabled: boolean value expected/),
      `Fails with invalid type: ${enabled}`
    );

would become

    await Assert.rejects(
       Emulation.setTouchEmulationEnabled({ enabled }),
       /enabled: boolean value expescted/,
      `Fails with invalid type: ${enabled}`
    );

Might be better to wait until Bug 1480075 is fixed first, so that failure reporting becomes more consistent for this helper.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That looks pretty neat and I feel could be a good mentored bug once the dependency has been fixed. Thanks for pointing that out Julian!

Comment 2

[LyScott123]

Hello! Is anyone working on this bug? If not may I start on this one? Thank you!

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to LyScott123 from comment #2)

Hello! Is anyone working on this bug? If not may I start on this one? Thank you!

Sure thing, I'll assign the bug to you then.

Comment 4

[Julian Descottes [:jdescottes]]

Hi LyScott123,

Just a quick nudge, let us know if there's anything we can do to help you get started with this.

Comment 5

[LyScott123]

Hey! So sorry! I'm using this bug to figure out why my tests aren't running. I got help on matrix and someone mentioned it was my SSH configuration. I tried a bunch of fixes and nothing worked, so I decided to redo the entire configuration process over now that I have more insight.

I finally have a big chunk of time to work on it today so hopefully I can run the tests and submit a patch!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

LyScott123, if you still have problems don't hesitate to join us on Matrix. You can find our channel at https://chat.mozilla.org/#/room/#webdriver:mozilla.org.

Comment 7

[LyScott123]

Attachment: Bug 1676736 - Use Assert.rejects to test exceptions in Remote browser chrome test r?jdescottes

Comment 8

[LyScott123]

That was quick! I actually got my tests to work!

Kind of forgot the submitting flow so let me know if I did anything wrong or need to clean anything up!

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This sounds great LyScott! As I've seen Julian already reviewed the initial patch and mentioned that there are other cases as well that need to be included. Do you think that you will have the time to finish off this patch? It would be great to get it landed. Thanks!

Comment 10

[LyScott123]

Hey! Sorry for the super late reply! Yeah I'm pretty busy during the week so I don't have much time to work on it. I still would like to work on this bug, but if I'm taking too long maybe I can drop off this one and pick a different one?

Also, the initial change and the example for this bug worked perfectly when I ran all the tests, but when I tried changing this file and run the tests, (https://searchfox.org/mozilla-central/source/remote/cdp/test/browser/browser_session.js) I get this error.

"The 'expected' argument was not supplied to Assert.rejects() "
"The 'expected' argument to Assert.rejects() must be a Regexp, function or object. "

Not really familiar with tests, so sorry if I am doing something wrong! I'll provide a screenshot once I'm able to get to my laptop. Let me know if I should reach out for some help on matrix or drop this bug! Thanks!

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No worries at all if it takes longer. You could still consider to push incremental updates to Phabricator with the moz-phab submit --wip command. That doesn't request review but we could see that there is activity. Maybe that could be an option?

Also regarding your issue I tested myself and added the following code:

  await Assert.rejects(
    client.send("Hoobaflooba"),
    /Invalid method format/,
    `Fails with invalid method format`
  );

It works and doesn't fail. So please compare to what you have used, and I'm sure that you can get it running.

Comment 12

[LyScott123]

Hey! Super sorry for the super late reply. I finally have a lot more time to work on this bug so thank you for being patient!

I tried your example and it worked perfectly, so thank you for that. I also made some changes and used the moz-phab commands you suggested, thanks!

I'll have a lot more time to work on this now a days so hopefully I can get it done soon!

Comment 13

[Julian Descottes [:jdescottes]]

Hi LyScott!

Just wanted to check if you needed some help after the last review? I realize it's a big number of tests to update, so if that helps, we could breakdown this in several smaller bugs, basically one per folder (eg one bug for tests under remote/shared/messagehandler/test/browser, one for tests under remote/cdp/test/browser/dom/, etc...).

Let us know if that would be more manageable for you.

Comment 14

[LyScott123]

Hey! Sorry this is taking so long! The query that you gave me on phrabicator,

https://searchfox.org/mozilla-central/search?q=catch+%28&path=remote%2F**%2Ftest%2Fbrowser%2F&case=false&regexp=false,

I have almost everything done! Just a few files that I'm a little confused about. I mentioned one of them on phabricator yesterday, not sure if you were able to see it. The other ones I'll mention here.

https://searchfox.org/mozilla-central/source/remote/cdp/test/browser/head.js#62
https://searchfox.org/mozilla-central/source/remote/shared/messagehandler/test/browser/browser_handle_command_retry.js

Those two files were a part of the query, but they look a little more complex than the other try/catch blocks. Should I be changing those too? If so, any guidance would help!

https://searchfox.org/mozilla-central/source/remote/cdp/test/browser/page/browser_createIsolatedWorld.js#79

That file I have everything pretty much done, but the assertEvents under noEventsWhenRuntimeDomainDisabled part is confusing me. I can't find any documentation about it, so not sure if I need to add it? The tests run fine if I exclude them.

If there are any files or folders that I'm missing please let me know! Sorry once again for the delay! Should have a lot more time to work on it now.

Comment 15

[Julian Descottes [:jdescottes]]

Thanks!

Replied on phabricator, and for your other questions:

https://searchfox.org/mozilla-central/source/remote/cdp/test/browser/head.js#62

You can ignore this one, my searchfox query had a few false positives. This one can't be moved to Assert.throws/reject.

https://searchfox.org/mozilla-central/source/remote/shared/messagehandler/test/browser/browser_handle_command_retry.js

They should be relatively similar to the other Assert.rejects you migrated. You can replace the try catch blocks by:

    await Assert.rejects(
      onBlockedOneTime,
      e => e.name == "AbortError",
      "Caught the expected abort error when reloading"
    );

For the second one, just move the info() call above the the await Assert.rejects and use the appropriate variable name.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f55c55c40c7
Use Assert.rejects to test exceptions in Remote browser chrome test r=jdescottes,webdriver-reviewers

Comment 17

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/6f55c55c40c7

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

LyScott123 thank you a lot for your contribution! It's great to see that we now have a way nicer way to check for assertions. In case you are intrested in contributing more please let us know here or directly on Matrix in our webdriver channel.