feat: run filtered debug tests in runner #25265

ZachJW34 · 2022-12-23T21:30:21Z

Closes [IATR](M1.0) Test runner filtering #24855

User facing changelog

na

Additional details

Allows the filtering of tests in the runner based on the debug run results driven by the runId query parameter.

This PR introduces the concept of a Cypress.testFilter that will allow the filtering of tests based on their titlePaths. For example, describe('s1', () => { it('t1', () => {}) }) would result in a titlePath of s1 t1. If Cypress.testFilter = ['s1 t1'], only this test fill be processed by mocha. Currently it only supports an array of titlePaths to filter by, but I could see this expanding in the future to cb/regex.

If there is an active Cypress.testFilter, tests that are not included behave like they never existed (pruned from the parsed suite/tests). If there is a suite/test .only that does not contain a matching test it will not influence the tests that matched. If there is a suite/test .only that is included, it will behave as normal.

Steps to test

The query for what tests are filtered is stubbed for now, so you can modify these results inside coreDataShape.ts. To apply the filter, click on a spec then add &runId=123 to the url and refresh the page.

How has the user experience changed?

Screen.Recording.2023-01-03.at.2.18.22.PM.mov

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-12-23T21:30:23Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

warrensplayer

Just a preliminary review. Most of my questions are around the mashing of the test filter fields into the Spec type. I am guessing you did that in order to get them to ride along with the Spec when it is passed into the runner. Would it be hard to pass the test filter through separately?

packages/app/src/runner/unifiedRunner.ts

cli/types/cypress.d.ts

packages/app/src/store/specs-store.ts

packages/reporter/src/header/header.tsx

lmiller1990

This wasn't as complex as expected, nice job figuring this out. Just some early bird comments.

packages/app/src/runner/event-manager.ts

packages/app/src/runner/useEventManager.ts

packages/app/src/store/specs-store.ts

lmiller1990 · 2022-12-26T02:26:47Z

packages/reporter/src/header/header.tsx

 }

-const Header = observer(({ appState, events = defaultEvents, statsStore }: ReporterHeaderProps) => (
+const Header = observer(({ appState, events = defaultEvents, statsStore, spec }: ReporterHeaderProps) => (


Not needing to do this now, but we really need to solve App <-> Command Log communication. Although this works for now, I see this becoming a pretty big mess. I don't have any great ideas, but some kind of unified way to bridge the Vue <-> React gap would be great. Let's keep this is mind, definitely open to solutions to this.

I looked at the M2 work and there is a lot more communication coming up. We do get reactivity from the mobxrunnerstore, maybe we can setup a system whereby any change made to a pinia store is relayed to the mobx store and wrapped with runInAction

packages/reporter/src/main.tsx

…ive test stats

ZachJW34 · 2022-12-27T23:50:04Z

@warrensplayer @lmiller1990 haven't addressed all the comments but did a pretty large refactor. Pulled the testFilter off the Spec object and I'm deriving the stats for the badge based on the total number of tests we encounter while parsing and the total amount of valid runnables

…onlys

cypress · 2022-12-29T17:22:42Z

Test summary

9301 • 0 • 905 • 0 • 7

Run details


Project	cypress
Status	Passed
Commit	`0fd3283`
Started	Jan 11, 2023 5:32 PM
Ended	Jan 11, 2023 5:48 PM
Duration	15:55 💡
OS	Linux Debian -
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/net_stubbing.cy.ts	3
1	network stubbing > intercepting request > can delay and throttle a StaticResponse
2	... > with `times` > only uses each handler N times
3	... > stops waiting when an xhr request is canceled
	e2e/origin/cookie_behavior.cy.ts	2
1	... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
2	... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
This comment includes only the first 5 flaky tests. See all 7 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

ZachJW34 · 2022-12-29T20:59:48Z

@emilyrohrbough added you as a reviewer as I'm not too experienced with driver code and could use another pair of eyes outside of the CT team!

emilyrohrbough · 2022-12-30T13:52:30Z

packages/driver/src/cypress.ts

@@ -194,6 +195,7 @@ class $Cypress {
    this.browser = config.browser
    this.platform = config.platform
    this.testingType = config.testingType
+    this.testFilter = config.testFilter


Setting this on the Cypress instance like this prob makes sense to run this faster, but a new run finishes on the cloud with updated results, how will this reflect here? I assume the user would need to navigate away from this spec and come back to get the updated test filter?

Also am I understanding the reason for adding this -- to filter down flakey (or is it failed) cloud test results? If that's the case can we name this accordingly? testFilter sounds like a user-provided filter value and doesn't indicate to me this value is driven from cloud data. Would be helpful for maintenance and change impact if this ever needed updated.

The idea was to keep the filtering somewhat generic, even though it has only one use case to start. I would think that the driver would not care why the tests were being filtered, just that it was asked to filter them. Our thoughts is that this could be expanded in the future to add more ways to filter tests.

I can see that...Though it would typically think filter being a regex or list of regex patterns. Which maybe that could be the case later one. The fact it's stored on the global Cypress instance vs state make it feel like this value will be "static" for this Cypress instance.

This request #4886, would likely use some of this logic if we chose to support them, which would be driven by user actions in the reporter and not necessarily create a new Cypress instance. I also feel like there was another request recently for selecting a single test in the command long to run in .only() mode for quick debugging - just couldn't find the issue.

I think generating a new Cypress instance would be the cleanest solution as we already have all the wiring for restarting a spec (spec file changed, hitting the refresh button). Might be less performant but restarting has never been too expensive to my knowledge. When we restart, we get another pass at the mocha suite which allows the testFilter to get applied again. This testFilter could be modified with each cycle, enabling cypress-io/cypress/issues/4886 by listening to an event capturing the run results and changing the filter based on what failed, then triggering a restart.

I could change the filter to be regex/callback rather than string array which would add more utility to it.

emilyrohrbough · 2022-12-30T14:04:08Z

packages/driver/src/cypress/runner.ts

+}
+
+const normalizeAll = (suite, initialTests = {}, setTestsById, setTests, getRunnableId, getHookId, getOnlyTestId, getOnlySuiteId, createEmptyOnlyTest, incrementFoundTestsBy) => {
+  pruneEmptySuites(suite, incrementFoundTestsBy)


To confirm, we will only prune in open mode correct? if so, can we add a comment to clarify and/or can we skip this all together?

Can we add some unit tests around this behavior? https://github.com/cypress-io/cypress/blob/3925ae062a9b9dca4cc9d61391ce5f6ff0525a8b/packages/driver/cypress/e2e/cypress/runner.cy.js

Not sure there is a good place to test this outside of the cy-in-cy test since the testFilter needs to be set before the spec file is executed.

We should be able to mock the testFilter value in unit tests and ensure the correct runnable results are returned.

packages/driver/src/cypress/runner.ts

emilyrohrbough · 2022-12-30T14:07:45Z

packages/driver/src/cypress/runner.ts

+}
+
+const pruneEmptySuites = (rootSuite, incrementFoundTestsBy) => {
+  for (const suite of [...rootSuite.suites]) {


Suggested change

for (const suite of [...rootSuite.suites]) {

if (!Cypress.testFilter) {

return // do nothing if the test filter is not set.

}

for (const suite of [...rootSuite.suites]) {

We still want to prune empty suites regardless of a testFilter being present or not so that the UI doesn't show the empty suite dropdowns.

Revisiting this, can I get your opinion on what you think the desired behavior is regarding empty suites e.g. describe('empty', () => {})?

Currently they show up in the UI as an empty runnable. Looks a bit wonky but has no side effects. With the way things are wired up now, these suites are pruned (regardless of if there is a test filter or not). What could be confusing is if a developer goes to test some new functionality and starts with describe.only('new test coverage', () => {}) and they hit save. Since the suite has no children, it will get pruned and the only will not apply. This behavior seems pretty confusing to me

I moved this to an earlier check, we won't call pruneEmptySuites unless there is a filter set.

I think this is a good start on this behavior. If we prune empty suites, we will end up with different run results in run mode which will remove the reporting of these empty suites to the dashboard. Even though there is nothing to "run" it does help visually indicate that a test has been stubbed out.

I do think it's confusing we have differences between empty suites vs empty test behaviors in the reporter visuals, but seems like a small fix in the reporter to show those consistently if we wanted to go that route.

packages/driver/src/cypress/runner.ts

emilyrohrbough · 2022-12-30T14:16:15Z

packages/reporter/cypress/e2e/header.cy.ts

+      cy.get('@debugDismiss').should('have.been.called')
+      cy.percySnapshot()
+    })
+  })


can we add more test cases when UI is squished and when there are large test cases to ensure the layout is as expected?

This test takes a curated runnables fixture. I could setup another fixture for filtered runnables but not sure what that would be testing since the filtering logic isn't being executed in this test.

I'm looking for coverage on how this wrap if there is a large number of test cases, say 24/200 tests. We don't have a lot of space in the header today, and wanted to make sure this is flexed/wrap. I know we had issues with this at one point

ped properly.

Added a test with snapshots for 4 different reporter widths to demonstrate wrapping

For reference, here's the wrapping behavior:

Screen.Recording.2023-01-10.at.3.42.00.PM.mov

emilyrohrbough · 2022-12-30T14:18:03Z

packages/reporter/src/runnables/runnables-store.ts

@@ -50,6 +50,7 @@ export class RunnablesStore {
   * content: RunnableArray
   */
  @observable runnablesHistory: Record<string, RunnableArray> = {}
+  @observable totalRunnables: number = 0


I'm not sure this needs to be observable. I'd expect this to be static.

I was under the assumption that it needs to be observable so it triggers a rerender on change. Though it is static once the run is parsed, the store is updated later

packages/app/src/runner/event-manager.ts

packages/app/src/store/specs-store.ts

packages/app/src/runner/event-manager.ts

packages/app/cypress/e2e/runner/cloud-debug-filter.cy.ts

emilyrohrbough · 2022-12-30T14:43:21Z

packages/driver/src/cypress/runner.ts

+}
+
+const normalizeAll = (suite, initialTests = {}, setTestsById, setTests, getRunnableId, getHookId, getOnlyTestId, getOnlySuiteId, createEmptyOnlyTest, incrementFoundTestsBy) => {
+  pruneEmptySuites(suite, incrementFoundTestsBy)


Can we add some unit tests around this behavior? https://github.com/cypress-io/cypress/blob/3925ae062a9b9dca4cc9d61391ce5f6ff0525a8b/packages/driver/cypress/e2e/cypress/runner.cy.js

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

ZachJW34 · 2023-01-03T20:02:15Z

@emilyrohrbough I've addressed your feedback. I refactored testFilter to no longer be on the Cypress global instance but rather explicitly passed to the mocha suite normalization. Setting Cypress.testFilter felt heavy handed as it's not in a place for external users to use and try to modify, and we might change the API later if we do want to open it up.

packages/app/src/runner/unifiedRunner.ts

packages/reporter/src/header/header.tsx

packages/reporter/src/header/DebugDismiss.tsx

packages/data-context/src/data/coreDataShape.ts

packages/reporter/src/header/DebugDismiss.tsx

…-debug-failures-filter

packages/app/src/runner/unifiedRunner.ts

emilyrohrbough · 2023-01-09T14:58:34Z

packages/app/src/runner/event-manager.ts

@@ -393,7 +397,7 @@ export class EventManager {

          this.studioStore.initialize(config, runState)

-          const runnables = Cypress.runner.normalizeAll(runState.tests, hideCommandLog)
+          const runnables = Cypress.runner.normalizeAll(runState.tests, hideCommandLog, testFilter)


Since this value is set in the specStore, any reason it's not directly pulled from here vs passing this around? useSpecStore.testFilter?

I'm not seeing anything that stands out to make me think this won't work correctly but have you by chance verified these filter changes with a spec that changes domains via cy.visit() mid-test? This would re-initialize Cypress mid-test and restore the reporter UI at the correct test index.

emilyrohrbough · 2023-01-09T15:02:20Z

packages/driver/src/cypress/runner.ts

+}
+
+const normalizeAll = (suite, initialTests = {}, testFilter, setTestsById, setTests, getRunnableId, getHookId, getOnlyTestId, getOnlySuiteId, createEmptyOnlyTest) => {
+  let totalUnfilteredTests = 0


should this default value be the total number of tests in the spec, not zero?

Thought looking at the UI, is this meant to represent the totalFilteredTests?

I believe Zach structured it this way to simplify accounting for filtered nested runnables (eg .only and "skipped due to browser" conditions). Was easier to sum up matching elements than it was to deduct non-matches

The variable naming appears correct to me - it's used for the "total" value in the output of Header.tsx (x/y tests)

emilyrohrbough · 2023-01-09T15:15:01Z

packages/driver/src/cypress/runner.ts

+      if (testFilter.includes(fullTitle)) {
+        tests.push(test)
+
+        if (rootSuite._onlyTests.includes(test)) {


we have quite a bit of logic to filter the "only" suites/tests later on in the normalize func. Is this needed here or can we rely on the existing filtering logic? if it is needed, do you know whats missing in the existing logic?

This preemptively filters out any runnables marked with .only that don't match the test filter before we get too far into parsing. We want the filter to take precedence, and without this we could end up including some tests that don't match the filter if they're marked with .only.

We are deleting the runnable references that don't match, I am surprised we are seeing tests incorrectly being included later on...

* Use specStore instead of passing `testFilter` as a param * Make type readonly

* Add test for responsive behavior of reporter panel with filter * Add test to validate filter is maintained across `cy.domain` reinitialization

mike-plummer · 2023-01-11T14:29:35Z

@warrensplayer @marktnoonan @emilyrohrbough All feedback thus far has been addressed save adding some unit tests for runner.ts. I will be looking at that this morning, but in the interim let me know if I missed anything or if there's any other feedback

mike-plummer · 2023-01-12T21:10:54Z

I'm really struggling to find a way to do more granular unit tests for the runner.ts stuff, but I'm not very familiar with this area so I may be overlooking something obvious. Any approach I take to mock out the testFilter from within a test doesn't apply in time or results in a loop of Cypress re-initializing. Any ideas are welcome, otherwise I may look at beefing up the cy-in-cy tests as a follow-on

warrensplayer · 2023-01-17T18:10:32Z

Merging this to the feature branch. Added new issue to address additional testing here: #25485

feat: run filtered debug tests in runner

354e6bd

ZachJW34 changed the title ~~feat: IATR-M0 Page Header (#24722)~~ feat: run filtered debug tests in runner Dec 23, 2022

warrensplayer reviewed Dec 23, 2022

View reviewed changes

packages/app/src/runner/unifiedRunner.ts Outdated Show resolved Hide resolved

cli/types/cypress.d.ts Outdated Show resolved Hide resolved

packages/app/src/store/specs-store.ts Outdated Show resolved Hide resolved

packages/reporter/src/header/header.tsx Outdated Show resolved Hide resolved

lmiller1990 reviewed Dec 26, 2022

View reviewed changes

fix only breaking the filter, refactor testFilter to be separate, der…

cfab8f5

…ive test stats

ZachJW34 added 3 commits December 28, 2022 13:41

e2e tests

fadb98a

post process mocha suite, remove empty suites and handle suite level …

97c7dcf

…onlys

feedback, add tests [run ci]

9074f63

ZachJW34 marked this pull request as ready for review December 29, 2022 18:15

ZachJW34 and others added 2 commits December 29, 2022 12:15

fix tests

98a336e

Merge branch 'feature/IATR-M0' into zachw/iatr-debug-failures-filter

6d18297

ZachJW34 requested review from brian-mann and emilyrohrbough December 29, 2022 19:29

emilyrohrbough reviewed Dec 30, 2022

View reviewed changes

packages/driver/src/cypress/runner.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Dec 30, 2022

View reviewed changes

marktnoonan and others added 3 commits December 30, 2022 14:42

Update cli/types/cypress.d.ts

e92339c

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

feedback: remove filter from Cypress global, consolidate runData

5e431a0

Merge branch 'feature/IATR-M0' into zachw/iatr-debug-failures-filter

ffa7fa2

remove commented code

07c3127

mike-plummer reviewed Jan 5, 2023

View reviewed changes

ZachJW34 added 2 commits January 5, 2023 14:30

Merge remote-tracking branch 'origin/feature/IATR-M0' into zachw/iatr…

97a14b6

…-debug-failures-filter

PR feedback

1900114

emilyrohrbough self-requested a review January 6, 2023 15:06

emilyrohrbough reviewed Jan 9, 2023

View reviewed changes

packages/app/src/runner/unifiedRunner.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Jan 9, 2023

View reviewed changes

packages/app/src/runner/unifiedRunner.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Jan 9, 2023

View reviewed changes

mike-plummer added 3 commits January 10, 2023 14:12

Address PR feedback

217a0dd

* Use specStore instead of passing `testFilter` as a param * Make type readonly

Add docs

3a59cc6

Address review comments

0fd3283

* Add test for responsive behavior of reporter panel with filter * Add test to validate filter is maintained across `cy.domain` reinitialization

warrensplayer mentioned this pull request Jan 17, 2023

IATR - M1.0 - Additional tests for Test Filtering #25485

Closed

warrensplayer merged commit 7d1d6e5 into feature/IATR-M0 Jan 17, 2023

warrensplayer deleted the zachw/iatr-debug-failures-filter branch January 17, 2023 18:11

This was referenced Jan 17, 2023

IATR M1.0 - Wire up test filtering backend call #25487

Closed

feat: wiring up test filters to the Debug page back end #25492

Merged

feat: run filtered debug tests in runner #25265

feat: run filtered debug tests in runner #25265

Conversation

ZachJW34 commented Dec 23, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Dec 23, 2022

warrensplayer left a comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 commented Dec 27, 2022 • edited Loading

cypress bot commented Dec 29, 2022 • edited Loading

Test summary

Run details

Flakiness

ZachJW34 commented Dec 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 commented Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

mike-plummer commented Jan 11, 2023

mike-plummer commented Jan 12, 2023

warrensplayer commented Jan 17, 2023

ZachJW34 commented Dec 23, 2022 •

edited

Loading

ZachJW34 commented Dec 27, 2022 •

edited

Loading

cypress bot commented Dec 29, 2022 •

edited

Loading

ZachJW34 Jan 3, 2023 •

edited

Loading

emilyrohrbough Dec 30, 2022 •

edited

Loading

emilyrohrbough Jan 9, 2023 •

edited

Loading

emilyrohrbough Dec 30, 2022 •

edited

Loading

ZachJW34 Jan 3, 2023 •

edited

Loading

ZachJW34 commented Jan 3, 2023 •

edited

Loading

emilyrohrbough Jan 10, 2023 •

edited

Loading