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

Can we please overwrite query commands #25078

Closed
bahmutov opened this issue Dec 8, 2022 · 24 comments · Fixed by #25674
Closed

Can we please overwrite query commands #25078

bahmutov opened this issue Dec 8, 2022 · 24 comments · Fixed by #25674
Assignees
Labels
E2E Issue related to end-to-end testing type: feature New feature that does not currently exist

Comments

@bahmutov
Copy link
Contributor

bahmutov commented Dec 8, 2022

What would you like?

Cypress v12 designated several query commands, and now prevents me from overwriting them. Which broke my plugins bahmutov/cypress-if#58 and bahmutov/cypress-aliases#17
Can we please allow overwriting queries, I promise to use it for good

Cypress.Commands.overwrite('as', ...)

Why is this needed?

I keep track of information or change behavior of several query commands in my plugins

Other

No response

@muratkeremozcan
Copy link

cypress-if and cypress-aliases are so core that I wish they were a part of the main code base.

@bahmutov
Copy link
Contributor Author

and cypress-map :)

@kostiantynvoiku
Copy link

kostiantynvoiku commented Dec 13, 2022

With the upgrade to v12 custom commands with Cypress.Commands.overwrite don't work now 😞

image

@estefafdez
Copy link

+1 to solve this issue soon, it's blocking us to be able to update to version 12. Thanks team!

@dpresiana

This comment was marked as duplicate.

1 similar comment
@salihcem

This comment was marked as duplicate.

@nagash77 nagash77 added type: feature New feature that does not currently exist E2E Issue related to end-to-end testing labels Dec 14, 2022
@BlueWinds
Copy link
Contributor

To everyone responding with "+1" - please use reactions on the issue rather than filling the issue with comments. We are discussing this and thinking about it.

@BlueWinds
Copy link
Contributor

One thing to keep in mind - if we add an API to overwrite queries, it will only be with other queries - there will be no replacing cy.get() with a non-query command. Something like cypress-if will still need to be written differently, rather than the current implementation of overriding .contains() / .get().

My suggestion for cypress-if specifically would be to look at other ways to achieve it that don't involve overwriting particular commands. My suggestion would be to look at either making .if() an assertion directly or having it insert an assertion ahead of itself in the command queue so that verifyUpcomingAssertions will bypass the implicit assertion check. This would also have the benefit of letting it follow any Cypress command, and not just two particular ones that are overridden.

@BlueWinds
Copy link
Contributor

Looking at cypress-aliases, much of it should still work. The exceptions are the overrides of .as and .contains. The .contains override looks like a decent usecase for overriding queries - will definitely take that into consideration.

I don't see that cypress-map needs to override anything? It looks like a very smooth migration to use queries instead of commands.

@sarahTheTester
Copy link

sarahTheTester commented Dec 14, 2022

What is the motivation for blocking overwrite of queries in the first place? I use query changes similar to Gleb's libraries, but now blocked.

Could it be made configurable by flag?

@emilyrohrbough
Copy link
Member

emilyrohrbough commented Dec 14, 2022

For those that would like to see this feature, can you please add a comment on why overriding a query is needed vs adding a custom query. We are interested in use-cases on why this is desirable beyond it preventing overriding existing queries for the sake of re-using the command name.

@BlueWinds
Copy link
Contributor

BlueWinds commented Dec 14, 2022

What is the motivation for blocking overwrite of queries in the first place? I use query changes similar to Gleb's libraries, but now blocked.

Could it be made configurable by flag?

We didn't block overrwriting so much as not add it - mostly due to time constraints. To support overwriting them will require a new API - Commands.overwrite can't work because queries are implemented differently internally. Even if we'd added a new API out of the box, it still would have required everyone (such as Gleb's plugins) to migrate.

As Emily said, we'd like to see the use cases people have, so when we do create a query overwrite API, we can make sure it's actually useful to people!

@mirobo
Copy link
Contributor

mirobo commented Dec 15, 2022

For those that would like to see this feature, can you please add a comment on why overriding a query is needed vs adding a custom query. We are interested in use-cases on why this is desirable beyond it preventing overriding existing queries for the sake of re-using the command name.

conditional testing

When testing frontend with live services, conditional testing is often required (or I should call it "conditionally closing irrelevant popups").

My solution still works with Cypress 12: https://github.com/mirobo/cypress-fiddle/blob/main/cypress/support/commands.js#L61-L88

But I'd like to see functionality like cypress-if added to cypress directly because @bahmutov 's solution blends in much better with cypress' syntax style and offers more features. It should be a core feature.

alias handling

the current possibilities with aliases and variables ( https://docs.cypress.io/guides/core-concepts/variables-and-aliases ) are leading to unreadable code when extensively used in RWA's.

// this table may contain a lot of data and we need to find a specific row where all 3 values are contained in a single row
cy.get('some-table').verifyRowContains([
  'expected data in column A',
  'expected data in column B',
  'expected data in column C'
])

// verifyRowContains returns the first row that contained all of the given values
.then((rowIndex) => {

  // we need to extract the values from each cell

  cy.get('some-table').getCellValue(rowIndex, 'column1').then((el) => {
    cy.wrap(el.text().trim()).as('valueFromColumnA');
  });
  cy.get('some-table').getCellValue(rowIndex, 'column2').then((el) => {
    cy.wrap(el.text().trim()).as('valueFromColumnB');
  });
  cy.get('some-table').getCellValue(rowIndex, 'column3').then((el) => {
    cy.wrap(el.text().trim()).as('valueFromColumnC');
  });
});

// we navigate to page that should display the same values as in the table
// but we do black box testing and we don't know/care where the data came from (it could actually be another service!)

cy.get('@valueFromColumnA').then((val) => {
  cy.get('someField1').should('contain', `FieldNameA: ${val as unknown as string}`);
});
cy.get('@valueFromColumnB').then((val) => {
  cy.get('someField2').should('contain', `FieldNameB: ${val as unknown as string}`);
});
cy.get('@valueFromColumnC').then((val) => {
  cy.get('someField3').should('contain', `FieldNameC: ${val as unknown as string}`);
});


// now imagine a case where one field contains all the three values concatenated in a special way and the ordering is relevant
// note that the previous .as(..)-commands happend in the same it-block, so we cannot access the values with "this.valueFromColumA"..

cy.get('@valueFromColumnA').then((valA) => {
  cy.get('@valueFromColumnB').then((valB) => {
    cy.get('@valueFromColumnC').then((valC) => {
      cy.get('someCombinedField').should('contain', `FieldNameABC: ${valA as unknown as string}, ${valB as unknown as string}: ${valC as unknown as string}`);
    });
  });
});

by using cypress-aliases, the callback hell can be eliminated completely:

  cy.get('someField1').should('contain', `FieldNameA: @valueFromColumnA`);
  cy.get('someField2').should('contain', `FieldNameB: @valueFromColumnB`);
  cy.get('someField3').should('contain', `FieldNameC: @valueFromColumnB`);

@bahmutov haven't tested if it works with multiple aliases... but it would be incredibly good!

cy.get('someCombinedField').should('contain', `FieldNameABC: @valA, @valB: @valC`); 

// maybe valC is a formatted amount we need to transform:
cy.wrap('@valC').then(formatAmount).as('valCformatted');
cy.get('someCombinedField').should('contain', `FieldNameABC: @valA, @valB: @valCformatted`); 

@kostiantynvoiku
Copy link

For those that would like to see this feature, can you please add a comment on why overriding a query is needed vs adding a custom query. We are interested in use-cases on why this is desirable beyond it preventing overriding existing queries for the sake of re-using the command name.

In my case, I overwrite .contains() command by adding regex for ignoring special symbols and pointing the default selector if none is provided.

@kryshenp
Copy link

kryshenp commented Dec 15, 2022

We have all our testing constants declared in a separate JSON file testing-constants.json, e.g.
{"ACCESS_LEVEL_DROPDOWN_BUTTON": "UIT_access_level_dropdown_button", "ACTIVE_ENTITY_INDICATOR": "UIT_active_entity_indicator",.......},

In commands.ts we check if it's usual selector or data-test-id:

const selectorOrDataTestId = (selector) => typeof selector == "string" && selector.startsWith("UIT_") ? `[data-test-id='${selector}']` : selector;

Then we have 3 commands overwritten:
Cypress.Commands.overwrite("get", (originalFn, selector, options) => { return originalFn(selectorOrDataTestId(selector), options); });

Cypress.Commands.overwrite("contains", (originalFn, subject, filter, text, options = {}) => { return originalFn(subject, selectorOrDataTestId(filter), text, options); });

Cypress.Commands.overwrite("find", (originalFn, subject, selector, options) => { return originalFn(subject, selectorOrDataTestId(selector), options); });

In the spec file we import values from testing-constants.json as TC and use it in the following way:

cy.contains(TC. ACCESS_LEVEL_DROPDOWN_BUTTON, "some string").find(TC. ACTIVE_ENTITY_INDICATOR).should("have.text", "validated");

Before Cypress 12 came out It was very comfortable for us to have it overwritten and not write steps like this:
cy.contains(`[data-test-id=`${TC. ACCESS_LEVEL_DROPDOWN_BUTTON}]`, "some string").find(`[data-test-id=${TC. ACTIVE_ENTITY_INDICATOR]}`).should("have.text", "validated")

or even

cy.contains("[data-test-id='UIT_access_level_dropdown_button']", "some string").find("[data-test-id='UIT_active_entity_indicator']").should("have.text", "validated")

I managed to create a new command for our custom cy.get() to make it work in Cypress 12, but having troubles with cy.contains() & cy.find().

@dylanesque
Copy link

My org relies on cypress-if in a project that tests certain scenarios using live data, and this change blocks that project from progressing beyond version 12.

@estefafdez
Copy link

Any update on this one?

@BlueWinds
Copy link
Contributor

BlueWinds commented Jan 4, 2023

Update is pretty straightforward - this is definitely something we want to support, and is next on my list after #25134 (which is a way larger change than it looks on the surface).

I don't have exact details on the what the API will look like yet, though. We're looking ahead a bit to design it in a way that will ultimately be compatible with the upcoming Async/Await API (which is the big project on my plate for this year).

I want to make sure that when we do provide an overwrite API for queries we won't have to change it a couple months down the line to support the next set of changes - this is why Cy12 didn't ship with it.

@JonTheStone
Copy link

Looking forward to getting this functionality back. We have overwritten cy.get so upgrading to v12 would mean a refactor of our entire test suite. Thanks!

@BlueWinds BlueWinds self-assigned this Jan 30, 2023
@BlueWinds
Copy link
Contributor

BlueWinds commented Jan 30, 2023

I'll be starting work on this this week.


Tentatively, the rules will work like this:

  1. Commands cannot be overwritten with queries.
  2. Queries cannot be overwritten with commands.
  3. Cypress.Commands.overwriteQuery(originalFn, ...userArgs) will be created, mirroring Cypress.Commands.overwrite(originalFn, ...userArgs).
  4. Inside overwriteQuery(), you can invoke queries but not other commands - exactly like any other query function. There is no way to add a .click() or .request() call inside of a query.

originalFn will be exactly the function that was passed into Cypress.Commands.addQuery() in the first place, with no wrapper or modification. Thus, if you call it, you'll get back the inner function.

Be aware that most queries rely on this - you'll want to use originalFn.call() / originalFn.apply().

Cypress.Commands.overwriteQuery('get', function newGet(originalFn, ...args) {
  console.log('overridden get')

  const innerGet = originalFn.apply(this, args)

  return (subject) => {
    const result = innerGet(subject)

    console.log('get() subject:', subject)
    console.log('get() result:', result)

    return result
  }
})

The above example adds a bunch of console.logs() to the execution of .get().

@estefafdez
Copy link

@BlueWinds hey! Any update on the status of this ticket? Being able to update to the latest cypress version is very important to us and we can't upgrade to V12 until this issue is fixed. Thanks!

@BlueWinds
Copy link
Contributor

The linked pull request is in review; we should have it merged in later this week, to go out with the next release.

@estefafdez
Copy link

Nice, thanks!! looking forward to have it and upgrade to the latest cypress version 🚀

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 15, 2023

Released in 12.6.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.6.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
E2E Issue related to end-to-end testing type: feature New feature that does not currently exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.