cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
47.52k stars 3.2k forks source link

8.2.0 and later still rerun before hooks when visiting multiple superdomains #17940

Open c32hedge opened 3 years ago

c32hedge commented 3 years ago

Current behavior

Previously mentioned in #14179, which was closed as a duplicate of #9026 but seems to have lost this distinction when that one was marked fixed by #17498. The behavior described below was not fixed by #17884.

I can confirm that #17498 fixed the issue with tests being skipped. However, as I described in #14179, the top-level before blocks still get run multiple times, once for each inner describe:

image

Note the extra log statements in the second and third tests--the before 1 and before 2 logs should only happen once, not three times.

Desired behavior

Before blocks shouldn't be rerun.

Test code to reproduce

describe('page', () => {
  before('before 1', () => {
    cy.log('before 1');
  });

  before('before 2', () => {
    cy.log('before 2');
  });

  describe('inner describe 1', () => {
    it('runs describe 1 test 1', () => {
      cy.visit('www.google.com');
      cy.log('describe 1 test 1');
    });
  });

  describe('inner describe 2', () => {
    it('runs describe 2 test 1', () => {
      cy.visit('www.cypress.io');
      cy.log('describe 2 test 1');
    });
  });

  describe('inner describe 3', () => {
    it('runs describe 3 test 1', () => {
      cy.visit('www.github.com');
      cy.log('describe 3 test 1');
    });
  });
});

Cypress Version

8.3.1

Other

Reproduced on both 8.2.0 and 8.3.1

If I replace the second and third cy.visit calls with the same superdomain as the first test, the tests behave as I would expect.

c32hedge commented 3 years ago

Well, looks like #17705 and #17935 were already entered for this. I'll go ahead and close as a duplicate, sorry for the noise but when I saw the issue my original was marked a duplicate of get closed as fixed, I assumed it meant both aspects had been fixed.

c32hedge commented 3 years ago

@Bkucera I'm reopening this because I can still reproduce my issue in Cypress 8.4.0, despite the merge of #17884.

c32hedge commented 3 years ago

@jennifer-shehane FYI

xianyiLuo commented 2 years ago

Cypress 9.2.0 encountered the same problem, can it be solved? @jennifer-shehane

cypress-app-bot commented 1 year ago

This issue has not had any activity in 180 days. Cypress evolves quickly and the reported behavior should be tested on the latest version of Cypress to verify the behavior is still occurring. It will be closed in 14 days if no updates are provided.

c32hedge commented 1 year ago

Hmmm, not sure I care for this bot. Bugs don't have a habit of just fixing themselves, and it's frustrating that the Cypress team never engaged or acknowledged the issue, despite the time and effort I put into creating a reproducible example. I can certainly try the MWE in my original description against the latest version of Cypress, but that doesn't feel worth my time unless a human from the Cypress team is willing to look at the issue.

nagash77 commented 1 year ago

Hi @c32hedge , Cypress has been evolving our support process in the last year. We are trying to do a better job of staying on top of issues both old and new. While we wish we had the resources and time to address every single issue that is raised by the community that is not realistic at this moment in time. The stalebot helps us make sure issues are still relevant, and in spite of what you might think, many bugs do get addressed either inadvertently or are missed when a developer is closing out related bugs.

The work Cypress did on cy.origin affected many areas of visit and how we handle domains in general. I would ask that you please verify that this issue is still happening. If you can confirm you are still seeing the issue, our team will be happy to pick this ticket back up and investigate.

colin0117 commented 1 year ago

Hi @nagash77 , I can verify this issue is still happening in 12.13.0 (Ubuntu).

before(() => {
    // Read-only tests so reset once
    cy.visit(Cypress.env('DT_DATABASE_RESET'));
    cy.wait(5000);
});

beforeEach(() => {
    cy.visit(Cypress.env('DT_EDITOR_URL') + '/examples/extensions/searchPanes.html');
});

describe('Editor tests - SearchPanes', () => {
    it('Single string in one pane', () => {
        cy.contains('Pandora').click();
    });

    it('Single string in two panes', () => {
        cy.contains('Singapore').click();
    });
});

In this code, the before() block is called before both the it() tests.

However, if I remove the cy.visit() in the before() block, then the before() is called only once as expected - so the cy.visit() is causing the issue..

AtofStryker commented 1 year ago

I was able to reproduce in with https://github.com/AtofStryker/cypress-issue-17940. The visit in the before() is definitely called twice when it should only be called once. I also think we might not be resetting top correctly here, but that may be a different issue

steve-schreiner commented 2 months ago

When will this be fixed? We just received email from a support engineer and they confirmed our issue even with the latest version. This is costing us 75K unneeded tests per year because of this. Thanks, Steve

jennifer-shehane commented 2 months ago

@steve-schreiner Cypress does not charge per 'hook' run, we only charge per 'it' block that is run. This should not cost you any extra in terms of extra tests run within Cypress. Could you explain how you calculated this costing 75K tests per year?

steve-schreiner commented 2 months ago

@steve-schreiner Cypress does not charge per 'hook' run, we only charge per 'it' block that is run. This should not cost you any extra in terms of extra tests run within Cypress. Could you explain how you calculated this costing 75K tests per year?

The work-around does indeed run in an 'it' block. We have added the duplicated 'it' blocks (91) times to resolve the problem in our test suite. Our test suite runs about 4 times a day which equates to an additional 95k unneeded tests per year.

image
jennifer-shehane commented 2 months ago

@steve-schreiner Thanks for describing the issue. I've added this as consideration for discussion for Cypress 14 since the change in this behavior, while more correct, would likely require some code changes from users. We'll be discussing scope of Cypress 14 soon and what should be included.

jennifer-shehane commented 1 month ago

Our team discussed this issue and made some implementation decisions. We will not make this a part of Cypress 14, since changing the behavior for all users might result in some unexpected behavior that users would not want.

To summarize:

So we need a switch to allow users to determine what behavior they want. We would introduce a new configuration, something like runBeforeOnEveryDomainChange that would default to true and users could opt this to `false.

Since this is no longer a part of Cypress 14 scope, we're prioritizing some 14 work first ahead of this.