Setting stubs/spies in before hooks and automatic restoring

pawelgalazka commented 7 years ago

Context

From Cypress docs:

Automatic reset/restore between tests

cy.stub() creates stubs in a sandbox, so all stubs created are automatically reset/restored between tests without you having to explicitly reset/restore them.

How this relates to stubs/spies which were set in before hooks ? I would expect that stub/spy which was in before won't be reset/restored for each test within the context. This seems to currently happening actually, but is that intended ?

bahmutov commented 7 years ago

@pawelgalazka could you give an example of a stub/spy that you set multiple stubs - some in before hooks, some in the test?

pawelgalazka commented 7 years ago

@bahmutov

describe('Google Analytics', () => {
  let gaStub

  before(() => {
    gaStub = cy.stub()
    cy.visit('http://localhost:8080', {
      onBeforeLoad: (contentWindow) => {
        Object.defineProperties(contentWindow, {
          'ga': {
            value: gaStub,
            writable: false
          }
        })
      }
    })
  })

  it('should perform logging on first page load', () => {
    cy.wrap(gaStub).should('have.been.called')
  })

  it('should report pageview', () => {
    cy.wrap(gaStub).should('have.been.called.with', 'send', 'pageview')
  })
})

This actually works, but following documentation, second test should fail, since stubs should be automatically restored/reset between tests. Is that correct ?

BTW. On subject page call window.ga('send', 'pageview') should be performed

chrisbreiding commented 7 years ago

The stub should be reset between tests regardless of where it's created, so this looks like a bug.

I was able to reproduce with this code, where the second test fails but should pass:

describe('stub not restored', function () {
  let stub
  before(function () {
    stub = cy.stub()
  })

  it('should be called', function () {
    stub()
    expect(stub).to.have.been.called
  })

  it('should not be called', function () {
    expect(stub).not.to.have.been.called
  })
})

The driver has undergone (and is still undergoing) a lot of refactoring for 0.20.0, so it's possible this has been fixed. I'll make sure to verify and add a test once the refactoring is finished.

pawelgalazka commented 7 years ago

I'm wondering if this should be treated as a bug, because if I set a stub in before hook I would actually expect that stub should not be reset between tests (at least for tests in the closest describe).

If stub is set in beforeEach then I would expect stub restoring between tests.

brian-mann commented 7 years ago

This brings up a lot of potential discussion points, but I think we are conflating many things which are actually separate and distinct from one another.

Point 1.

It is true that Cypress will reset stubs, aliases, routes, local storage, cookies, and virtually all state from the browser in between tests. We do not recommend sharing state, and we need to come up with fully fledged lifecycle API's to control this. It is not obvious nor easy to do this, and that's why it doesn't exist yet.

Point 2.

In your examples above, I'm see the correct behavior. Resetting a stub simply means to replace the existing method that was stubbed with the original.

In your example you're pulling out the stub reference to make to accessible in the entire closure. This reference is just that - a reference to the stub. You can continue to make assertions on it because its a legitimate stub. When it was 'reset', it was simply removed from being the function defined on windodw.ga. So the difference is that when your page calls window.ga it will no longer call the stub - but that's a different problem altogether.

Point 3.

I think our team kind of understands what you're doing. You're wanting to visit once, and then write a bunch of assertions in different tests about the thing you performed once. This conflicts with how we reset state between tests. What you're essentially writing is one giant test. There's not really any benefit splitting things out, because each test is already coupled to the others.

pawelgalazka commented 6 years ago

I’ve just want to address point 3 to explain the benefit of before hook. As you describe what I do here are kind of “passive asserts”, so I visit page once and then do bunch of asserts. Same can be for other example:

describe('when logged as admin', () => {
  before(() => {
      // some login commands
    cy.visit('http://localhost:8080/some/page')
  })

  it('should display panel bar', () => {
    cy.get('.panel-bar').should('be.visible')
  })

  it('should should show additional item', () => {
    cy.get('.admin-items').should('be.visible')
  })
})

There could be a case that additional item could be shown but admin panel bar not at this state (because of some weird error with permissions checks in template), so still there is a benefit of splitting this up to have a better feedback, where we fail exactly.

Then there is a question, why not to use beforeEach then ? Well if you have 30 passive tests like that and you visit staging site env, every visit is time expensive. Suddenly test execution jumps from 10s to 5 minutes. True tests share state, but each test does not perform any action which could potentially change it. So we gain nothing with beforeEach in this case, except longer tests execution.

Because of this it would be great if it would be possible to disable automatic alias/stubs clearing for certain tests/test files to be able to use them with before hooks.

Hope this explains situation better.

pacop commented 6 years ago

Somes news right here?

Because of this it would be great if it would be possible to disable automatic alias/stubs clearing for certain tests/test files to be able to use them with before hooks.

This idea rocks!

moroshko commented 4 years ago

Any news here? @pawelgalazka Did you find a reasonable workaround?

pawelgalazka commented 4 years ago

@moroshko unfortunately I haven't found any workarounds this

Sumanth1703 commented 4 years ago

Any update or possible work around for this ?

jonyw4 commented 3 years ago

Sorry to bother you guys, but I think that it's a very unexpected behavior, do you have some idea when a fix will come?

TheGooner93 commented 3 years ago

+1 . this would be a welcome feature!

melibe23 commented 2 years ago

I will definitely enjoy this enhancement. I use an alias to save the Google Analytics events therefore I must use a beforeEach and this makes the script last longer. Which is a very very very disadvantage on E2E testing.

I tried in every situation to assert several things on one single test. But IMO this approach applies very well for assertions that do not require interaction, for example, SEO checks. onClick events, by nature, need an interaction. So, to properly cover each of them I have to create different tests, one per interaction.

Imagine a component with 3 onClick links, I lose testing coverage if I try to use several assertions on one test so I can use a before.