cy.request + Callback Queue Race Conditions prevent LocalStorage from being cleared prior to each `it`

[egucciar]

Current behavior:

When i have a complex set of conditions (100% reproduced in the Test repository below 👇 ), localStorage is not cleared prior to each it (we do a cy.visit in beforeEach)

One weird condition - cy.request MUST be called before visit and it MUST take some time to respond (I've found as little as 1s can cause failures, but in this example i set my mock api to 2s)

Another weird condition - the application code has to be updating the localStorage in a weird way. In my example, I use redux with redux-localstorage and a setTimeout which increases on each iteration.

We assert localStorage item redux_test is null and it fails:

it is 100% reproducible.

Desired behavior:

If we take out the cy.request in the beforeEach, every time it is run, the localStorage is cleared correctly even with all the wacky setTimeout redux stuff done in app code (no other app code changes)

Desired behaviour is, Even when doing cy.request in beforeEach, the output should be all passing:

Steps to reproduce:

You can clone this Repository https://github.com/egucciar/cypress-support/tree/local-storage

On the local-storage branch (linked above)

Run:

git checkout local-storage
yarn
yarn start

In another terminal run:

yarn cypress:open

Observe errors. Also comment out the cy.request to observe passing.

Versions

Cypress v3.1.0, Chrome 70, MacOS 10.13.1

Notes

My mocklab API account will have its free trial expired within the next 2 weeks i think, so please take a look ASAP~~

This has been plaguing us forever and i FINALLY Have a standalone reproducer that is 100% reliable 🎉 😭

[egucciar]

@brian-mann

This is the local storage not being cleared issue you asked me if i could reproduce 1 month ago...Just looking for some support here considering my mock api lab trial will run out within a week and change :) (The repoducer is dependent on a cy.request which takes 1-2s long to respond)

[jennifer-shehane]

Hey @egucciar, I am getting an error when running yarn start. Could you resolve this issue and let me know when it's updated?

ERROR in ./src/index.js
Module not found: Error: Can't resolve 'redux' in '/Users/jennifer/Dev/cypress-support/src'
 @ ./src/index.js 1:0-45 22:14-25
 @ multi (webpack)-dev-server/client?http://localhost:8080 ./src/index.js

ERROR in ./src/index.js
Module not found: Error: Can't resolve 'redux-localstorage' in '/Users/jennifer/Dev/cypress-support/src'
 @ ./src/index.js 2:0-45 7:17-29
 @ multi (webpack)-dev-server/client?http://localhost:8080 ./src/index.js
ℹ ｢wdm｣: Failed to compile.

[egucciar]

@jennifer-shehane did you run yarn after checking out the branch? each branch in this repo is a mini-reproducer with its own dependencies! I just double checked and redux (and redux-localstorage) is in the package.json!

[egucciar]

@jennifer-shehane apologies for the confusion. I just updated the master branch to make it clear that is the "skeleton" for reproducers and link-outs to specific reproducers. And I updated local-storage branch to include, in the How to section, git checkout local-storage in the first step, before yarn!

[jennifer-shehane]

I pulled down the repo again from scratch and ran the instructions. Am getting a new error now.

 yarn start
yarn run v1.10.1
$ webpack-dev-server --mode development
/bin/sh: webpack-dev-server: command not found
error Command failed with exit code 127.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Usually I find these situations are a difference between node or npm versions from different users, so if there's a specific version of these I need to use - let me know.

[egucciar]

@jennifer-shehane i did not change the Repo but this command worked for you before (note how the webpack ran but was missing the dependencie)

ERROR in ./src/index.js
Module not found: Error: Can't resolve 'redux' in '/Users/jennifer/Dev/cypress-support/src'
 @ ./src/index.js 1:0-45 22:14-25
 @ multi (webpack)-dev-server/client?http://localhost:8080 ./src/index.js

ERROR in ./src/index.js
Module not found: Error: Can't resolve 'redux-localstorage' in '/Users/jennifer/Dev/cypress-support/src'
 @ ./src/index.js 2:0-45 7:17-29
 @ multi (webpack)-dev-server/client?http://localhost:8080 ./src/index.js
ℹ ｢wdm｣: Failed to compile.

Now you are getting an error that webpack-dev-server command is not found

Since the only thing i changed was readme here ...i am thinking your yarn step may not have executed successfully. On second look i can see we added cypress-helpers to dependencies & this is a private repo. So running yarn on the branch failed for you

I have removed the private dependency. please try to yarn once again after doing a git pull!!

[jennifer-shehane]

Yes, yarn did error before from the private dep, but I missed it. I'm not as familiar with what errors/success looks like in yarn.

yarn start is successful, but yarn cypress:open is now not successful

$ cypress open
/bin/sh: cypress: command not found
error Command failed with exit code 127.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[egucciar]

@jennifer-shehane I do not understand why "cypress" command is not found.

"cypress": "^3.1.0",

Is in devDependencies...

[jennifer-shehane]

Apparently I had to run yarn within each tab. So, first tab

yarn
yarn start

Second tab:

yarn 
yarn cypress:open

It's running now.

[egucciar]

@jennifer-shehane that is weird and unexpected. I've never had to do that.

[egucciar]

Thanks for your patience :)

[jennifer-shehane]

New error in the Cypress runner. 😅

[egucciar]

@jennifer-shehane sorry my repo was messed up because it didnt get cleaned up!!!! Its fixed now git pull

[egucciar]

@jennifer-shehane my bad for not cleaning up the left overs from other reproducers...it's why i made master clean now :) but it was all hindsight after this was working and pushed up (maybe my old node modules also stuck around which gave me the false sense of security that it was working)

[jennifer-shehane]

I cleaned up some of the test code, just so the value is not being passed around, so we get the localStorage within each test.

describe('Local Storage Reproducer', () => {
  beforeEach(() => {
    // comment out below for tests to now be passing!
    cy.request({
      method: 'POST',
      url: 'http://nlv4d.mocklab.io/json2',
    }).should((resp) => {
      expect(resp.status).to.eq(200)
    })
    cy.visit('/');
    cy.get('#test');
  })
  context('testing', () => {
    it('1', () => {
      cy.window().then((win) => {
        expect(win.localStorage.getItem('redux_test')).to.be.null;
      })
    })
    it('2', () => {
      cy.window().then((win) => {
        expect(win.localStorage.getItem('redux_test')).to.be.null;
      })
    })
  })
});

This fails on both 1 and 2. I'm not totally understanding where and why you are expect localStorage to be null at the time the test begins.

This also fails regardless of whether the cy.request() is commented out or not.

What is it in your code that is setting this object in LocalStorage?

[egucciar]

@jennifer-shehane ...well by changing the stucture of my test, you remove the reproducer...we need to assert on the value at onBeforeLoad time....but cannot run assertions in the onBeforeLoad. The assertions are there to show it fails.

but anyway. LocalStorage needs to be cleared before the test runs, else we are getting weird failures in our application code because we expect localStorage to be cleared by cypress as you guys say it does ....and our application code expects a "clean state" in each IT but is not getting any...

By storing the value in the onBeforeLoad call, we can see it passes the first time, but not the subsequent tiems. The value should be null in the onBeforeLoad call. But its not.... & this failure is not reproduced if cy.request is removed.

[egucciar]

@jennifer-shehane See brian explaining what Cypress does before each test:

How I Got here:

We would expect then, when visiting via cy.visit between its the "stale" localStorage is not persisted between tests.. .however you can see from my perfectly valid reproducer that the localStorage value DOES exist in the onBeforeLoad from the previous test.

Check the src/index.js for the App Code. Basically the idea i had was to try to simulate the "complexity" of an application by modifying localStorage asyncronously in the app code. Its nothing special. Just a bunch of setTimeouts and redux.

Theoretically, I'm not doing anything that typical apps wouldn't do. But for just some odd reason, when we have a cy.request in the beforeEach, sometimes localStorage just....sticks around.

We noticed this first a month back when one of our tests started failing all of a sudden. On further inspection it was clear to me that failure was due to some state being persisted from one it to the next it. Then these failures started happening in other projects in my org. Sessions persisting between runs and failing our sign in code (because we were already signed in...).

So i made it a priority to get a reproducer up so that we wouldnt be doing onBeforeLoad: (win) => win.localStorage.clear() or removeItem where item was the specific app code sticking around. This is gross and "Anti cypress" so i wanted to boil down our Apps to the minimum reproducible Test code.

I did that, and found that cy.request was the weak link, and then further boiled down the specific request with a mockLab API call to find out what specifically about the request caused our app to fail (it had to be at least 1s).

Then, this was still using our app code. So i had to try to create some App code that updates localStorage "similarly" to how App code does. I attempted many different ways of setting localStorage. First vanilla, then by asking myself what apps did. I did this first by using Redux (though we use Redux in one project, and Apollo in another project, both have this issue). Then i messed with the timeouts and everything until i was able to get the localStorage to be "persisted" between ITs.

What i want:

It would be really nice if we could remove the win => win.localStorage.clear() from our onBeforeLoad in our app code. Please let me know if this proves that there is a race condition in the Cypress framework which prevents it from truly clearing our localStorage state between its.

[jennifer-shehane]

K, then this is a proper reproducible example. You can add expectations in the beforeEach. This does pass when commenting out the cy.request() and fails on the 2nd test's beforeEach when the cy.request() is present.

Sorry for the back and forth. I was just trying to get the lowest reproducible example.

describe('Local Storage Reproducer', () => {
  beforeEach(() => {
    // comment out below for tests to now be passing!
    cy.request({
      method: 'POST',
      url: 'http://nlv4d.mocklab.io/json2',
    }).should((resp) => {
      expect(resp.status).to.eq(200)
    })
    cy.visit('/', {
      onBeforeLoad: (win) => {
        expect(win.localStorage.getItem('redux_test')).to.be.null;
      }
    });
    cy.get('#test');
  })
  context('testing', () => {
    it('1', () => {
      cy.window().then((win) => {
        expect(win.localStorage.getItem('redux_test')).to.be.object;
      })
    })
    it('2', () => {
      cy.window().then((win) => {
        expect(win.localStorage.getItem('redux_test')).to.be.object;
      })
    })
  })
});

I believe this may be related to this issue -> of an asynchronous function from a previous test setting localStorage after the test is done. We will fix this issue in a later release.

Workaround

Manually clear localStorage in the onBeforeLoad in cy.visit(). The tests pass when below is added.

cy.visit('/', {
  onBeforeLoad: (win) => {
    win.localStorage.clear()
    expect(win.localStorage.getItem('redux_test')).to.be.null;
  }
});

[egucciar]

Thanks Jennifer!

I believe this may be related to this issue -> of an asynchronous function from a previous test setting localStorage after the test is done. We will fix this issue in a later release.

Once this release is done, we can update this reproducer to confirm it is 100%.

Thanks for boiling down the Reproducer some more, I will update the Repo. We have the work around in our code but it's still very brittle. For instance, we use a cy.request to set our session in localStorage, so our onBeforeEach needs to just remove our apollo app data and not the whole thing in some projects, whereas in others we need it to clear elsewhere. Currently it takes a lot of mental overhead + dev hours to figure out the "perfect storm" of conditions of when we should clear our storage and what should be cleared. We are constantly bending our brains as new issues pop up and figuring out what we need to do, since cy.clearLocalStorage() isnt a catch-all for this issue and we need to rely on the visit while also making sure we dont clear stuff we needed from before the visit :)

I noticed that other issue, but one thing they said was that returning API calls are the ones writing to tests after they complete. Since that sounded good, but needed to be proven, I went ahead and tried to make a really reliable reproducer (which doesnt rely on any API calls within the App, but theoretically similar due to setTimeouts and varying async-ness in the app code)

I also dont know if the "size" of localStorage affects anything. It's also bizarre that cy.request is causing failures, and without its passing 100%

I'm glad I was able to provide a reproducible example of this behaviour. Thanks for the tip about running assertions in onBeforeLoad (I'm guessing we can't run cy. commands there which is where i got messed up). I'll update my reproducer for clarity to match yours.

[egucciar]

@jennifer-shehane when you have a minute could you update the labels on this issue? thanks!

[cars10]

Thanks you @egucciar for this bug report. I was facing the same issue in my project (with basically the same setup, just with vue + vuex + vuex-persistedstate) and i have spent ages trying to figure out was is wrong in my specs. +1

[vladyslav-tecorien]

Its been few months after this issue had been found. Is there any progress on fixing this issue? @jennifer-shehane

[jennifer-shehane]

No work has been done on this issue.

[killthekitten]

@egucciar thanks for the effort to make the reproducer available! I'm going over your code right now, and the http://nlv4d.mocklab.io/json2 is long dead. I plugged in a different endpoint, but out of curiosity, what was there before?

[egucciar]

@killthekitten it was just a rest API, I couldn't reproduce with normal async work

[killthekitten]

@egucciar makes sense, thanks!

@jennifer-shehane could this be caused by a silenced exception here? The try/catch block was added a while ago when the cross origin errors were first handled. This would also explain why was it impossible to reproduce without a call to an external domain.

I was able to reproduce the issue using @egucciar's code, but I'm not sure how to test my hypothesis. Ideally, I'd like to run their code with an "editable" version of cypress, and see what's going on. Could you give me a hint where to start, maybe?

[killthekitten]

After a closer look, it seems a race condition indeed.

The local storage cleanup happens in the test:before:run listener: https://github.com/cypress-io/cypress/blob/1690d41d763704d18f6ab638f9a19a910cd70266/packages/driver/src/cy/commands/local_storage.js#L25-L33

A bit down the line, the window object gets updated during the window:before:load event: https://github.com/cypress-io/cypress/blob/1690d41d763704d18f6ab638f9a19a910cd70266/packages/driver/src/cypress/cy.js#L1194-L1198

Which is in turn triggered from the proxy inject code: https://github.com/cypress-io/cypress/blob/1690d41d763704d18f6ab638f9a19a910cd70266/packages/proxy/lib/http/util/inject.ts#L13-L22

So, If there was a cross origin request, it sets both events (test:before:run and window:before:load) apart in time, which lets us reliably reproduce the bug. At this point, cleanup happens earlier than it should, only cleaning the window object of the previous test (are they different objects though?). In fact, removing the cy.request code and running the tests with an open console somehow gives a similar effect.

It looks like the reproducer tests pass only when window:before:load is triggered early enough to update the reference to window before the local storage cleanup. However, I couldn't confirm this with a console.log.

This comment describes something similar: https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cypress/cy.js#L919-L926

[jennifer-shehane]

If you’re encountering something similar to this issue, please update to the latest version of Cypress. We made a lot of changes, in particular in v12. Please open a new issue if you’re experiencing something similar in a newer version.