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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsdom update 5 days ago breaks back compatability #6798

Closed
vlad0 opened this Issue Aug 2, 2018 · 20 comments

Comments

Projects
None yet
@vlad0
Copy link

vlad0 commented Aug 2, 2018

馃悰 Bug Report

jest.spyOn(localStorage, "setItem"); doesn't work;

jest-environment-jsdom has caret dependency on jsdom -> 11.5.1, jsdom latest version is 11.12.0 and it includes localStorage and sessionStorage. Previously we could mock localStorage easily but now we don't have that option and we need to use the default localStorage implementation. So our option is to spy on it which doesn't work because it throws TypeError: object[methodName].mockImplementation is not a function

What I have discovered is that in ./node_modules/jsdom/lib/jsdom/living/generated/Storage.js the issue is with the set method of the Proxy

Currently the code below is on line 278.

if (ownDesc === undefined) {
          const parent = Reflect.getPrototypeOf(target);
          if (parent !== null) {
            return Reflect.set(parent, P, V, receiver);
          }
          ownDesc = { writable: true, enumerable: true, configurable: true, value: undefined };
        }

if we remove receiver from return Reflect.set(parent, P, V, receiver); we will be able to spy on it. But I guess that's coming from webidl converter

To Reproduce

Steps to reproduce the behavior:

  1. Install jest.
  2. Write a simple class that leverage localStorage.setItem() or localStorage.getItem()
  3. Try to spy on it -> jest.spyOn(localStorage, "setItem"); and it will throw an error

Expected behavior

The method should be available for spying.

Link to repl or repo (highly encouraged)

https://github.com/vlad0/jest-localstorage-issue

Run npx envinfo --preset jest

  1. npm install
  2. npm test
    Paste the results here:
> jest

 FAIL  ./service.spec.js
  Service
    鉁 Service set value (7ms)

  鈼 Service 鈥 Service set value

    TypeError: object[methodName].mockImplementation is not a function

       8 |
       9 |     it('Service set value', () => {
    > 10 |         jest.spyOn(localStorage, "setItem");
      11 |         service.setValue("hello", "world")
      12 |
      13 |         expect(localStorage.setItem).toHaveBeenCalled();

      at ModuleMockerClass.spyOn (node_modules/jest-mock/build/index.js:597:26)
      at Object.it (service.spec.js:10:14)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        0.785s, estimated 1s
Ran all test suites.
npm ERR! Test failed.  See above for more details.

@vlad0 vlad0 changed the title localStorage update came from jsdom jsdom update 5 days ago breaks back compatability Aug 2, 2018

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Aug 2, 2018

Not much for jest to do here, I'm afraid... You can always rollback jsdom and use a lockfile

@vlad0

This comment has been minimized.

Copy link
Author

vlad0 commented Aug 3, 2018

@SimenB , to be honest I was really confused where to raise this issue but since jest-environment-jsdom is part of this repo and it has caret dependency to jsdom which causes the issue I decided to give it a try here.

Let me give another perspective:

Imagine I am a new Jest user and want to spy on localStorage - What are our options to spy on localStorage with the latest version of Jest? - after all it downloads the latest version of jsdom which can't be spied on or mocked.

There are lots of example with mocking localStorage before jsdom implementation but no solution after the lastest jsdom update.

UPDATE: jsdom team suggests the fix should come from jest :) so we seem to be in the limbo :)
jsdom/jsdom#2318

@mr-wildcard

This comment has been minimized.

Copy link

mr-wildcard commented Aug 14, 2018

@vlad0 did you manage to mock/spy anything from jsdom's localStorage implementation ?

Since jsdom released its 11.12.0, if I recall, there is no workaround for this :/ kind of a blocker when you want to unit test...

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Aug 14, 2018

Like they said in the jsdom issue, how would this be mocked in the browser? If it's impossible to mock in the browser we might be at an impasse. There is nothing we can do inside Jest that you cannot do in userland, but if you're able to mock it using plain JSDOM then we can look into porting the solution into Jest

@mr-wildcard

This comment has been minimized.

Copy link

mr-wildcard commented Aug 14, 2018

I understand the pov of @domenic on the jsdom side, and I agree this should not be fixed by them.

Maybe I'm missing something, but how comes that I can jest.spyOn on JSON.parse for example, and not on localStorage's methods ?

I'm getting the same error than @vlad0 :

TypeError: object[methodName].mockImplementation is not a function

Edit : with Jasmine's global spyOn it's working..

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Aug 14, 2018

I added a comment over there. Jsdom has different behaviour from chrome - the function is not possible to replace by simple assignment, which is what Jest does (or tries to do) see

object[methodName] = this._makeComponent({type: 'function'}, () => {
object[methodName] = original;
});
object[methodName].mockImplementation(function() {
return original.apply(this, arguments);
});
. Line 766 is the one that throws since the assignment on line 762 silently fails

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Aug 14, 2018

Ok, that's apparently a bug in Chrome. If anyone is able to somehow replace a function on localStorage in Safari (or Edge), then we can look into porting that over to Jest so it can handle it automatically.

I'll send a PR throwing an error when trying to spy on something that's not replacable

@mr-wildcard

This comment has been minimized.

Copy link

mr-wildcard commented Aug 14, 2018

We found out that localStorage from Jest is an instance of a class called Storage provided by jsdom.
And using spyOn this way works :

jest.spyOn(Storage.prototype, 'setItem');
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Aug 14, 2018

Spying on the prototype is an interesting solution.

@thymikee @rickhanlonii thoughts on falling back to trying to spy on the prototype? My local diff is this:

diff --git i/packages/jest-mock/src/index.js w/packages/jest-mock/src/index.js
index 9b679c73e..cedbe376a 100644
--- i/packages/jest-mock/src/index.js
+++ w/packages/jest-mock/src/index.js
@@ -763,6 +763,10 @@ class ModuleMockerClass {
         object[methodName] = original;
       });
 
+      if (object[methodName] === original) {
+        throw new Error(`Unable to mock method \`${methodName}\``);
+      }
+
       object[methodName].mockImplementation(function() {
         return original.apply(this, arguments);
       });

and a test, but in theory we could try some more fancy stuff by looking up constructors etc

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Aug 14, 2018

What if your obj/fn overrides the prototype? You'll end up mocking different functions, no?

@moimael

This comment has been minimized.

Copy link

moimael commented Aug 15, 2018

@mr-wildcard Thanks a lot for the workaround ! How would you distinguish between local and sessionStorage though ? I'd like to check that my data is saved in the right storage area in my tests.

@tamlyn

This comment has been minimized.

Copy link

tamlyn commented Aug 15, 2018

Something to bear in mind is now that jsdom provides a working localStorage implementation, you may no longer need to mock it. I just update my tests to assert on the value retrieved via getItem instead.

@mr-wildcard

This comment has been minimized.

Copy link

mr-wildcard commented Aug 15, 2018

@moimael good question, I didn't think about sessionStorage. I can't give it a try right now but if both sessionStorage and localStorage are instantiated from a generic Storage class, then it could be problematic to distinguish them... 馃

@tamlyn yes, but in this case you're not unit testing your code anymore because localStorage is not mocked and by doing that way you now need to take care of clearing mutated state with afterEach beforeEach & co. Which is not really the kind of isolation in which I want to test some of my business code.

@th3fallen th3fallen referenced this issue Sep 6, 2018

Closed

Error on latest version on Jest (23.5.0) #88

3 of 3 tasks complete

prymitive added a commit to prymitive/karma that referenced this issue Oct 3, 2018

prymitive added a commit to prymitive/karma that referenced this issue Oct 3, 2018

@tomprats

This comment has been minimized.

Copy link

tomprats commented Nov 10, 2018

Here's a solution I adapted from a similar problem mocking window.location #5124

describe("sessionStorage", ()=>{
  let originalSessionStorage;

  beforeAll(()=>{
    originalSessionStorage = window.sessionStorage;
    delete window.sessionStorage;
    Object.defineProperty(window, "sessionStorage", {
      writable: true,
      value: {
        getItem: jest.fn().mockName("getItem"),
        setItem: jest.fn().mockName("setItem")
      }
    });
  });

  beforeEach(()=>{
    sessionStorage.getItem.mockClear();
    sessionStorage.setItem.mockClear();
  });

  afterAll(()=>{
    Object.defineProperty(window, "sessionStorage", {writable: true, value: originalSessionStorage});
  });

  it("calls getItem", ()=>{
    sessionStorage.getItem("abc");

    expect(sessionStorage.getItem).toHaveBeenCalledWith("abc");
  });
});
@omalraj

This comment has been minimized.

Copy link

omalraj commented Nov 22, 2018

jest.spyOn(window.localStorage.__proto__, 'setItem'); should work without any additional imports.

@mr-wildcard

This comment has been minimized.

Copy link

mr-wildcard commented Jan 17, 2019

@corey-aloia-sap

This comment has been minimized.

Copy link

corey-aloia-sap commented Jan 18, 2019

@mr-wildcard I am so embarrassed that I had to delete my comment. It actually looks like a big plus that we can't overwrite this, otherwise someone might accidentally create a mock with read instead of getItem :).
jest.spyOn(Object.getPrototypeOf(window.localStorage), 'getItem');
Works great, and is, IMO, an improvement over the older version of Jest/JSDom where we had to do this manually :)

@j18s

This comment has been minimized.

Copy link

j18s commented Jan 29, 2019

How about this, what do you guys think about below code pluses and deltas:

const localStorage = jest.fn(); localStorage.getItem = () => 'abc....'; console.log(localStorage.getItem());

similarly we can set the item .... I haven't tried setItem yet but will try ...

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 29, 2019

If people can come up with some way we can handle this generically in jest-mock, I'm happy to reopen. As is, I don't think this is actionable, so I'm closing this. Please keep discussing, though!

@SimenB SimenB closed this Jan 29, 2019

@maneeshpal

This comment has been minimized.

Copy link

maneeshpal commented Feb 21, 2019

technically, Storage.prototype.setItem is exactly the same as localStorage.__proto__.setItem.
So, mocking localStorage.__prop__.setItem doesn't mean you are targeting localStorage.
This mock will also apply for any sessionStorage.setItem calls within your unit of test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.