1405240 - Error message assertions in test_cookie.js

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Peter Major]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

Changes made to the error message assertions in test_cookie.js, for example:

    Assert.throws(
        () => cookie.add({name: "name", value: invalidType}),
        "Foobar");

do not take any effect, and the tests will keep on passing.


Actual results:

The tests passed even though the returned error message was different from the expectation.


Expected results:

The tests should have failed.

Comment 1

[Peter Major]

To run the test, execute:
./mach test testing/marionette/test_cookie.js

Comment 2

[Peter Major]

I will try to investigate this issue in the next few days.

Comment 3

[Peter Major]

Attachment: Bug 1405240 - Fix test_cookie.js unit test assertions

When verifying error messages, the second parameter of Assert.throws has
to be a regular expression.

Review commit: https://reviewboard.mozilla.org/r/186076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186076/

Comment 4

[Peter Major]

Comment on attachment 8914798 [details]
Bug 1405240 - Fix test_cookie.js unit test assertions

https://reviewboard.mozilla.org/r/186076/#review191072

::: testing/marionette/cookie.js:116
(Diff revision 1)
>  
>    if (restrictToHost) {
>      if (newCookie.domain !== restrictToHost) {
>        throw new InvalidCookieDomainError(
>            `Cookies may only be set ` +
> -          ` for the current domain (${restrictToHost})`);
> +          `for the current domain (${restrictToHost})`);

This change was part of Bug 1402978 already, hopefully merge conflict resolution in mercurial will take care of this at the merge.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8914798 [details]
Bug 1405240 - Fix test_cookie.js unit test assertions

https://reviewboard.mozilla.org/r/186076/#review191104

Good find!  If you want to continue contributing you might want to
check the other testing/marionette/test_*.js files if they also use
strings.

If you plan to submit more patches to Gecko, I would happily request
commit access level 1 for you so that you can trigger your own try
runs in the Mozilla CI called Treeherder:

	https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2f39b4ff11&group_state=expanded

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8534a2a4b5fe -d 8129c4382245: rebasing 423843:8534a2a4b5fe "Bug 1405240 - Fix test_cookie.js unit test assertions r=ato" (tip)
merging testing/marionette/cookie.js
merging testing/marionette/test_cookie.js
warning: conflicts while merging testing/marionette/test_cookie.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Andreas Tolfsen ❲:ato❳]

It looks like Mercurial is not clever enough to resolve the conflict
itself.  I’m pretty sure git would be able to in this case…

Do you mind rebasing once your other change hits mozilla-central,
and I will re-land this?

Comment 8

[Peter Major]

Comment on attachment 8914798 [details]
Bug 1405240 - Fix test_cookie.js unit test assertions

https://reviewboard.mozilla.org/r/186076/#review191104

I went through the output of `grep -nr 'Assert.throws' -A 2 testing/marionette` and couldn't spot any other occurrences.

With regards to access: I'm happy to have access to try builds, but wouldn't want to have commit rights just yet. I'm very much a rookie on mercurial and probably could learn lot more there.

// On the other hand I can barely navigate through MozReview (had to read the manual to reply :)), but I'm sure there are more excellent dev resources that can help me with try builds too.

Comment 9

[Peter Major]

Comment on attachment 8914798 [details]
Bug 1405240 - Fix test_cookie.js unit test assertions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186076/diff/1-2/

Comment 10

[Andreas Tolfsen ❲:ato❳]

(In reply to majorpetya from comment #8)

> With regards to access: I'm happy to have access to try builds,
> but wouldn't want to have commit rights just yet. I'm very much a
> rookie on mercurial and probably could learn lot more there.

I have filed https://bugzil.la/1405668 to request level 1 access
for you.  Note that there are a few steps you need to take that are
outlined in the guidelines.

With regards to version control, it is also possible to use git for
Gecko development.  I do this myself.  You can see the workflow
tutorial for how to:

	https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development

If you have any questions I’m known as ato on the irc.mozilla.org
server.  Marionette development is mostly discussed in the #ateam
channel.

> // On the other hand I can barely navigate through MozReview (had
> to read the manual to reply :)), but I'm sure there are more
> excellent dev resources that can help me with try builds too.

mozreview has a terrible UI and I’m informed it will be replaced
“soon”.  Whether that replacement is any better I don’t know.

The other alternative is to upload your .patch files directly to the
bug.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b4e006bc55a
Fix test_cookie.js unit test assertions r=ato

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2b4e006bc55a