Error message assertions in test_cookie.js

RESOLVED FIXED in Firefox 58

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: majorpetya, Assigned: majorpetya)

Tracking

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Component: Untriaged → Marionette
Product: Firefox → Testing
(Assignee)

Comment 1

2 years ago
To run the test, execute:
./mach test testing/marionette/test_cookie.js
(Assignee)

Comment 2

2 years ago
I will try to investigate this issue in the next few days.
Assignee: nobody → majorpetya
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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
Attachment #8914798 - Flags: review?(ato) → review+

Comment 6

2 years ago
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)
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?
Flags: needinfo?(majorpetya)
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(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

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b4e006bc55a
Fix test_cookie.js unit test assertions r=ato
(Assignee)

Updated

2 years ago
Flags: needinfo?(majorpetya)

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b4e006bc55a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Attachment #8914798 - Flags: review?(hskupin)
You need to log in before you can comment on or make changes to this bug.