fix: Reject with real Error instances like Firefox does #293

rpl · 2021-03-11T11:32:33Z

Same as #258, re-created to double-check if circle CI would run the CI job as expected.

Fixes #257

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

rpl · 2021-03-11T18:17:00Z

@Rob--W would you mind to continue your review in this PR?
we couldn't figure out why CircleCIi is refusing to run the CI job on #258, and on the other PR that @fregante did try to open to workaround the issue in #258.

Rob--W · 2021-03-12T14:00:08Z

test/fixtures/detect-existing-browser-api-object/content.js

+      t.notOk(chrome.runtime.lastError instanceof Error);
+    });
+
+    t.end();


Does this t.end(); call cause the test runner to pause until the callback for chrome.storage.local.set has been called?

I used t.plan in f6aece9 to ensure the number of assertions is the same for both browsers

Rob--W · 2021-03-12T14:00:50Z

test/fixtures/detect-existing-browser-api-object/content.js

@@ -10,12 +10,27 @@ test("browser api object in content script", (t) => {
    // On Firefox, window is not the global object for content scripts, and so we expect window.browser to not
    // be defined.
    t.equal(window.browser, undefined, "window.browser is expected to be undefined on Firefox");
+
+    try {


These tests don't really belong to this test. Could you put them in their own location?

Same file, different test(), right?

Done in f6aece9

Rob--W · 2021-03-12T14:01:24Z

test/fixtures/detect-existing-browser-api-object/content.js

+      await browser.storage.sync.set({a: 'a'.repeat(10000000)});
+      t.fail('It should throw when attempting to set an object over quota');
+    } catch (error) {
+      t.ok(error instanceof Error);


Is it possible to check the error message? This condition is true for almost every error, including "not defined" etc.

Done in f6aece9

fregante · 2021-03-13T01:14:48Z

@rpl can you pick f6aece9 or give me push access to your fork?

rpl · 2021-03-15T10:38:06Z

@rpl can you pick f6aece9 or give me push access to your fork?

just saw this and I did cherry pick f6aece9 into this branch (and I did also invite you in my fork, to make this easier if you'll need to push more changes into this branch).

rpl · 2021-03-15T10:50:13Z

@fregante the test case is currently failing because at the moment the actual Error message in Firefox is not the same as the error in Chrome:

 error types (Firefox)
    ✖  should be equal
    -------------------
        operator: deepEqual
        diff: "'QUOTA_BYTES quota exceeded'" => "'The storage API will not work with a temporary addon ID. Please add an explicit addon ID to your manifest. For more information see https://mzl.la/3lPk1aE.'"
        source: assert@moz-extension://34eb5c09-f75b-46a0-84d4-49dd697a71da/tape.js:9607:54
        bound@moz-extension://34eb5c09-f75b-46a0-84d4-49dd697a71da/tape.js:9459:32
        equal@moz-extension://34eb5c09-f75b-46a0-84d4-49dd697a71da/tape.js:9768:10
        bound@moz-extension://34eb5c09-f75b-46a0-84d4-49dd697a71da/tape.js:9459:32
        @moz-extension://34eb5c09-f75b-46a0-84d4-49dd697a71da/content.js:45:9

We are mainly interested that the error rejected is instanceof Error that not as much as we are interested into the two error messages being exactly the same, I see that Rob asked to assert the error message in the previous review round and so I propose to just use a different expected error message for Chrome and Firefox while asserting the expected error message.

We could also add the addon id in the test fixture extension manifest (by adding the applications.gecko.id property to the manifest), but the error message will still be slightly different form the one raised on Chrome, in Firefox the error message should be this one: "QuotaExceededError: storage.sync API call exceeded its quota limitations."

and so I think that for the purpose of the test we could also just opt to expect the "The storage API will not work with a temporary addon ID..." error message and don't add an explicit addon id in the test extension manifest.

fregante · 2021-03-15T10:51:15Z

I think I know what's happening with CircleCI 😂

I registered to be able to see the results (it doesn't even let you look without it) but then I removed the CircleCI from my GitHub account.

So probably they aren't building my commits out of spite.

fregante · 2021-03-15T10:52:14Z

and so I think that for the purpose of the test we could also just opt to expect the "The storage API will not work with a temporary addon ID..." error message and don't add an explicit addon id in the test extension manifest.

Good idea. I'll try fixing the CI issue and push that fix.

rpl · 2021-03-15T10:52:53Z

I think I know what's happening with CircleCI joy

I registered to be able to see the results (it doesn't even let you look without it) but then I removed the CircleCI from my GitHub account.

So probably they aren't building my commits out of spite.

ah I see, 🤦 .... that isn't a nice behavior from circleci, it didn't even show anything to let us understand that

rpl · 2021-03-15T10:54:08Z

and so I think that for the purpose of the test we could also just opt to expect the "The storage API will not work with a temporary addon ID..." error message and don't add an explicit addon id in the test extension manifest.

Good idea. I'll try fixing the CI issue and push that fix.

thank you! ping me if CI is still refusing to run CI jobs on your commits and I'll force update this branch as a last resort to force it

This reverts commit 620996b.

fregante · 2021-03-15T10:57:12Z

Sorry for the noise, Chrome integration tests fail locally SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 87

Rob--W

r+wc

Rob--W · 2021-03-16T16:46:30Z

test/fixtures/detect-existing-browser-api-object/content.js

+test("error types", async (t) => {
+  if (navigator.userAgent.includes("Firefox/")) {
+    try {
+      await browser.storage.sync.set({a: 'a'.repeat(10000000)});


Based on the error message, it seems that you can remove the keys themselves. The API call is rejected without even trying to store the data.

rpl · 2021-03-17T11:43:40Z

test/fixtures/detect-existing-browser-api-object/content.js

+  if (navigator.userAgent.includes("Firefox/")) {
+    try {
+      await browser.storage.sync.set({a: 'a'});
+      t.fail('It should throw when attempting to set an object over quota');


Nit, the assertion message doesn't match the actual error scenario now, we could change it to t.fail('storage.sync should throw when called from an extension with a temporary id') or something like that.

(@fregante I can make the change and update this branch if you don't get to it first, in the end it is just a small nit related to an assertion message and so I don't mind if you prefer that I make the change and then land this on master).

Minor tweak to one of the assertion messages applied in edaa539.

fregante · 2021-03-18T19:14:13Z

philipp-tailor · 2021-04-09T13:50:09Z

Great change! 🚀

Are there plans to publish a new release?

And, if not: Is mozilla:master stable? If so, I might change my dependency to pull from the branch instead.

rpl · 2021-04-09T15:00:06Z

Great change! rocket

Are there plans to publish a new release?

Yes, we should be publishing it soon, I have been busy with other projects and I didn't got to come back to this (I'll mainly need to review the changes that we will release, generate a changelog and agree with Rob if we should release it as a new major version, which I think it may be reasonable based on some of the changes that I recall)

And, if not: Is mozilla:master stable? If so, I might change my dependency to pull from the branch instead.

We run integration tests on each change and so it should be fairly stable, but there may be some changes in behavior that may be worth to keep into account (well, one is this one, which you are already aware of).

Reject with real Errors like Firefox does

97acf68

rpl mentioned this pull request Mar 11, 2021

Reject with real Errors like Firefox does #258

Closed

rpl requested a review from Rob--W March 11, 2021 18:15

Rob--W requested changes Mar 12, 2021

View reviewed changes

Extract tests to their own test() + assert message

e195877

Enable Storage API in test extension

620996b

fregante added 2 commits March 15, 2021 04:55

Revert "Enable Storage API in test extension"

c8417be

This reverts commit 620996b.

Use Firefox error message

d58113c

fregante added 2 commits March 15, 2021 04:58

Learn to count errors

12c3cee

Do away with callbacks; Use promises

b175c90

Sorry for the noise, Chrome integration tests fail locally SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 87

rpl requested a review from Rob--W March 16, 2021 16:28

Rob--W approved these changes Mar 16, 2021

View reviewed changes

Simplify error-thrower for Firefox

8b524c4

rpl commented Mar 17, 2021

View reviewed changes

chore: Minor tweak to one assertion message

edaa539

rpl merged commit 5ab825b into mozilla:master Mar 18, 2021

philipp-tailor mentioned this pull request Apr 9, 2021

Wrapped errors from sendMessage should include the original error name, stacktrace #210

Open

ramzimalhas mentioned this pull request Aug 21, 2024

[Snyk] Upgrade webextension-polyfill from 0.7.0 to 0.12.0 ramzimalhas/refined-zapier#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Reject with real Error instances like Firefox does #293

fix: Reject with real Error instances like Firefox does #293

rpl commented Mar 11, 2021 •

edited

Loading

rpl commented Mar 11, 2021

Rob--W Mar 12, 2021

fregante Mar 13, 2021

Rob--W Mar 12, 2021

fregante Mar 13, 2021 •

edited

Loading

Rob--W Mar 12, 2021

fregante Mar 13, 2021

fregante commented Mar 13, 2021

rpl commented Mar 15, 2021

rpl commented Mar 15, 2021

fregante commented Mar 15, 2021

fregante commented Mar 15, 2021

rpl commented Mar 15, 2021

rpl commented Mar 15, 2021

fregante commented Mar 15, 2021

Rob--W left a comment

Rob--W Mar 16, 2021

rpl Mar 17, 2021

rpl Mar 18, 2021

fregante commented Mar 18, 2021

philipp-tailor commented Apr 9, 2021

rpl commented Apr 9, 2021

fix: Reject with real Error instances like Firefox does #293

fix: Reject with real Error instances like Firefox does #293

Conversation

rpl commented Mar 11, 2021 • edited Loading

rpl commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Mar 13, 2021

rpl commented Mar 15, 2021

rpl commented Mar 15, 2021

fregante commented Mar 15, 2021

fregante commented Mar 15, 2021

rpl commented Mar 15, 2021

rpl commented Mar 15, 2021

fregante commented Mar 15, 2021

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Mar 18, 2021

philipp-tailor commented Apr 9, 2021

rpl commented Apr 9, 2021

rpl commented Mar 11, 2021 •

edited

Loading

fregante Mar 13, 2021 •

edited

Loading