Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject with real Errors like Firefox does #258

Closed
wants to merge 2 commits into from
Closed

Reject with real Errors like Firefox does #258

wants to merge 2 commits into from

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Jan 23, 2021

Fixes #257

Apart from cross-browser compatibility, this also follows this suggestion https://eslint.org/docs/rules/prefer-promise-reject-errors

@fregante fregante changed the title Correct lastError type in tests Correct lastError type Jan 23, 2021
@fregante fregante marked this pull request as draft January 23, 2021 21:34
@fregante fregante marked this pull request as ready for review January 23, 2021 21:54
@@ -56,7 +56,7 @@ describe("browser-polyfill", () => {
it("rejects the returned promise if chrome.runtime.lastError is not null", () => {
const fakeChrome = {
runtime: {
lastError: new Error("fake lastError"),
lastError: {message: "fake lastError"},
Copy link
Contributor Author

@fregante fregante Jan 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the correct type returned by Chrome.

The style used here matches other errors in the file.

Comment on lines +74 to +75
instanceOf(err, window.Error, "Expected the error to be an instance of Error");
equal(err.message, fakeChrome.runtime.lastError.message,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
instanceOf(err, window.Error, "Expected the error to be an instance of Error");
equal(err.message, fakeChrome.runtime.lastError.message,
equal(err, new window.Error(fakeChrome.runtime.lastError.message),

I had also tried the above but it kept saying that expected [Error: …] to equal [Error: …] even if err.constructor === window.Error. Maybe it compared other properties as well, not just .message

@fregante fregante changed the title Correct lastError type Reject with real Errors Jan 23, 2021
@fregante fregante changed the title Reject with real Errors Reject with real Errors like Firefox does Jan 23, 2021
@Rob--W
Copy link
Member

Rob--W commented Jan 25, 2021

Thanks for the PR. We will discuss this during the next triage meeting on Thursday next week.

The change in the test that replaces the Error instance with a plain object looks good to me (as it matches Chrome's actual behavior).

I'm not sure if it is desirable to replace a plain error object with an Error instance, but I have no strong opinions on either direction.

@rpl
Copy link
Member

rpl commented Feb 4, 2021

Thanks for the PR. We will discuss this during the next triage meeting on Thursday next week.

We discussed about this today during our triage meeting and agreed that the issue this PR aims to fix is in-scope for the polyfill and we are ok on proceeding with this PR.

Rob will cover the reviews, but I do have one comment related to the test coverage:

This kind of changes should also be covered with an integration test, because that is how we are making sure that the behavior of the polyfill when running on Chrome is "as similar as possible" to the behavior the same scenario would have in Firefox (where the polyfill will be just a no-op), and to catch with a test failure when that is not the case anymore (e.g. because of changes introduced in new Firefox or Chrome versions).

The fixture extensions that are being used for the integration tests are in this directory:

Some other details (e.g. how to run the integration test locally on a single "browser type") are in this section of the CONTRIBUTING.md file:

@fregante
Copy link
Contributor Author

fregante commented Mar 8, 2021

I added some tests and they pass in Chrome locally, but Firefox tests aren't running locally and they're not passing. Do you see anything wrong there?

@Rob--W
Copy link
Member

Rob--W commented Mar 8, 2021

I added some tests and they pass in Chrome locally, but Firefox tests aren't running locally and they're not passing. Do you see anything wrong there?

What are you running and what are you observing? Any logs?

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, the CI job didn't run the test on chrome because it is complaining that the chromedriver version used isn't compatible with the chrome version used in the CI workers.

I see that in your branch the chromedriver version is 87, while on master is 88 (and I'm going to merge the version 89 in a minute), would you mind to rebase the PR as part of the next update on this PR so that the test will be able to run on both the browser also in the CI job?

@rpl
Copy link
Member

rpl commented Mar 10, 2021

@fregante I'm not sure why, but the last update pushed to this PR didn't trigger the circle CI job.
It may have been a temporary issue, I don't see any errors on the circle CI side, and I tried to ask renovatebot to rebase another PR and the CI job did start as expected.

would you mind to force push again to see if that triggers the CI job as expected?

@fregante fregante mentioned this pull request Mar 10, 2021
@fregante
Copy link
Contributor Author

fregante commented Mar 10, 2021

I rebased the branch on master, nenzi ncora :(

@rpl
Copy link
Member

rpl commented Mar 11, 2021

@fregante bah, I really can't see what is going on with circle ci with your branch (nothing in it should prevent circle ci from running a job for it).

I just tried to pull your branch locally, rebased it, squashed the fixup on the first patch and then created a PR from that branch pushed in my fork (#293)... and that (for the Murphy's law) did work just fine... sigh

Do you want to try to pull the branch from #293 and force push it in yours or recreate a new PR from it?
otherwise we may just ask @Rob--W to do a final review pass on #293, if there are no other changes needed we may just merge that one.

Let me know wdyt (and my apologies on behalf of circle ci ;-))

@fregante fregante mentioned this pull request Mar 11, 2021
@fregante
Copy link
Contributor Author

Feel free to merge your PR. I tried deleting my fork and re-sending another PR, still nothing

@fregante fregante closed this Mar 11, 2021
@rpl
Copy link
Member

rpl commented Mar 11, 2021

@fregante I'm sorry, I can't really think of any reasons why it should trigger a build (especially given that it did built it before and I have even double-checked that the third party PR are enabled on the CI side).

I'll put #293 up for review from Rob.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors aren't instances of Error
3 participants