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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
c32hedge opened this issue Aug 31, 2021 · 14 comments
Open
Labels
E2E Issue related to end-to-end testing Reproducible Can be reproduced Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team.

Comments

@c32hedge
Copy link

c32hedge commented Aug 31, 2021

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
Copy link
Author

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
Copy link
Author

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

@c32hedge
Copy link
Author

c32hedge commented Oct 1, 2021

@jennifer-shehane FYI

@xianyiLuo
Copy link

xianyiLuo commented Jan 12, 2022

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

@cypress-app-bot
Copy link
Collaborator

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.

@cypress-app-bot cypress-app-bot added the stale no activity on this issue for a long period label May 15, 2023
@c32hedge
Copy link
Author

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
Copy link
Contributor

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.

@nagash77 nagash77 self-assigned this May 24, 2023
@cypress-app-bot cypress-app-bot removed the stale no activity on this issue for a long period label May 24, 2023
@colin0117
Copy link

colin0117 commented May 29, 2023

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
Copy link
Contributor

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

@AtofStryker AtofStryker added Reproducible Can be reproduced Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team. E2E Issue related to end-to-end testing labels May 31, 2023
@AtofStryker AtofStryker removed their assignment May 31, 2023
@steve-schreiner
Copy link

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
Copy link
Member

@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
Copy link

@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
Copy link
Member

@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.

@mschile mschile self-assigned this Oct 10, 2024
@jennifer-shehane jennifer-shehane moved this from Understanding to Designing / Scoping in Cypress App Priorities Oct 11, 2024
@mschile mschile removed the Cypress 14 Issues scoped for Cypress 14 label Oct 21, 2024
@mschile mschile removed their assignment Oct 21, 2024
@jennifer-shehane
Copy link
Member

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:

  • Some things in before hooks are not good to run twice ever - like setting up a database
  • Some things in before hooks need to run on the new domain once it's visited because it would have no affect otherwise since it deals with the application under test.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2E Issue related to end-to-end testing Reproducible Can be reproduced Triaged Issue has been routed to backlog. This is not a commitment to have it prioritized by the team.
Projects
None yet
Development

No branches or pull requests

9 participants