fix: add top-level run-all button to allow all specs to be run at once #24846

ZachJW34 · 2022-11-28T18:17:50Z

Closes Allow running all specs without root directory #24759

User facing changelog

experimentalRunAllSpecs: Allow running all specs regardless of folder structure by adding a top-level run-all button.

Additional details

We only allowed running "all specs" if the user happened to have one root folder e.g. storing all specs under cypress/e2e. If they had two root folders containing specs, this wasn't possible.

There is now a "top-level" run all button in both the Specs Page SpecsList and the Runner SpecsList.

DRAFT: Need to verify InlineSpecList button with design.

Steps to test

Checkout this branch, choose a project and add experimentalRunAllSpecs to the e2e config. Run yarn watch and yarn cypress:open in two separate terminals and then choose your project. Verify the Specs page and the Runner has the top-level "Run All Specs" button and that it functions correctly.

How has the user experience changed?

Screen.Recording.2022-11-28.at.12.16.18.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)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?

cypress-bot · 2022-11-28T18:17:52Z

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.

lmiller1990 · 2022-11-29T01:33:27Z

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

+}
+`
+
+const isRunMode = window.__CYPRESS_MODE__ === 'run' && window.top === window


We've got something like

const isRunMode = window.__CYPRESS_MODE__ === 'run'

In quite a few places - any chance we can move this to a single function?

Can do, I think the window.top === window is for cy-in-cy? Makes sense to consolidate that as well, going to include that and see if anything breaks.

Addressed in b86fb64

lmiller1990 · 2022-11-29T01:36:15Z

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

+
+const isRunMode = window.__CYPRESS_MODE__ === 'run' && window.top === window
+
+export const useRunAllSpecsStore = defineStore('runAllSpecs', () => {


Should we be consistent and use the "options" style store that we use for our other stores? Or is there a limitation preventing us from using that style here?

Only way to execute the gql query without having to call an explicit action. I didn't want to convert to using the store but I would have had to refactor the InlineSpecsList to get the communication to work. I'll think on this a bit more as I'm not too happy with the implementation.

I'm still not entirely clear why this isn't possible using the options style store, but happy to move forward as is. I also feel this implementation isn't ideal, but I can't really nail down why - other than the fact we had to touch so many files to implement it.

Anyway, we can add this to our mental list of "things to improve" if/when we get a chance to attack some of the technical debt. It would be nice if the stores could be all written in a consistent style, though - what's the blocker on the options style store?

lmiller1990 · 2022-11-29T01:38:52Z

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

+
+  return {
+    isRunAllSpecsAllowed,
+    allSpecs: allSpecsRef,


Seems allSpecs is not used outside the store; a private ref - should we not return it here (thus not exposing it)?

You're right! It became unused after a refactor

Edit: Addressed in b86fb64

lmiller1990

Did not test it out -- still in draft -- just left a few comments.

…n-all-backup

cypress · 2022-11-29T21:00:50Z

Test summary

46553 • 0 • 4326 • 0 • 31

Run details


Project	cypress
Status	Passed
Commit	`3e0a325`
Started	Dec 1, 2022 3:13 PM
Ended	Dec 1, 2022 3:32 PM
Duration	18:39 💡
OS	Linux Debian -
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	e2e/origin/cookie_behavior.cy.ts	4
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)
3	... > 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
4	... > 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)
	top-nav-launchpad.cy.ts	1
1	... > global mode > shows "continue" button after login if project selected
This comment includes only the first 5 flaky tests. See all 31 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

lmiller1990

I left some comments, no obvious blocker, mostly style/architectural things.

packages/app/cypress/e2e/run-all-specs.cy.ts

packages/app/src/pages/Specs/Runner.vue

lmiller1990 · 2022-11-30T04:49:32Z

packages/app/src/specs/InlineSpecList.cy.tsx

@@ -207,18 +190,43 @@ describe('InlineSpecList', () => {
  })

  describe('Run all Specs', () => {
+    const hoverRunAllSpecs = (directory?: string, specNumber?: number) => {


This is only called in two places and both directory and specNumber are always defined. We can remove the ?. It also means line 199 is never executed.

packages/app/src/specs/InlineSpecListHeader.vue

lmiller1990 · 2022-11-30T04:57:04Z

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

+
+const isRunMode = window.__CYPRESS_MODE__ === 'run' && window.top === window
+
+export const useRunAllSpecsStore = defineStore('runAllSpecs', () => {


I'm still not entirely clear why this isn't possible using the options style store, but happy to move forward as is. I also feel this implementation isn't ideal, but I can't really nail down why - other than the fact we had to touch so many files to implement it.

Anyway, we can add this to our mental list of "things to improve" if/when we get a chance to attack some of the technical debt. It would be nice if the stores could be all written in a consistent style, though - what's the blocker on the options style store?

lmiller1990 · 2022-11-30T05:00:24Z

According to the issue linked, the label should be "Run N Specs" when using the filter. Eg:

This should be "Run 2 specs". Did that requirement change? I might have missed it.

The actual feature is working as expected. :100

astone123 · 2022-11-30T17:27:35Z

Worked well for me, just waiting on the answer to Lachlan's comment - seems like a missing requirement

mike-plummer

Works great! Just one question about expected behavior:

In Inline, if you search for a spec that doesn't exist you can still attempt to do a "Run All" on an empty set of specs. It's gracefully handled (gives the "no tests found" message), but I wonder if it makes more sense to disable the "Run All" control if the inline list is empty

mike-plummer · 2022-11-30T19:39:45Z

packages/app/src/specs/InlineRunAllSpecs.vue

@@ -22,7 +24,7 @@
        class="font-normal text-sm inline-flex"
        data-cy="tooltip-content"
      >
-        {{ t('specPage.runAllSpecs', specNumber) }}
+        {{ specNumber === 'all' ? t('specPage.runAllSpecs') : t('specPage.runSelectedSpecs', specNumber) }}


Could we just use RUN_ALL_SPECS_KEY instead of the "all" magic string?

I'll go ahead and make this change - I think it makes sense, and Zach W looks to be out for a day.

Turns out we don't need this anymore, instead we just show the number as recommended by design: "Run N specs".

mike-plummer · 2022-11-30T19:42:41Z

packages/app/src/specs/InlineSpecList.vue

@@ -80,4 +83,6 @@ const specs = computed<FuzzyFoundSpec[]>(() => {
  return fuzzySortSpecs(specs, debouncedSpecFilterModel.value)
 })

+const runAllSpecsStore = useRunAllSpecsStore()


nit: Thoughts on just destructuring here to simplify the template expressions above?

const { isRunAllSpecsAllowed, runAllSpecs } = useRunAllSpecsStore()

Reactive get stale when destructured from the pinia store, which is why I've generally avoided doing it. There's a storeToRefs helper we can use if we want. But I ended up kind of liking that when we use reactive values from the store, it's clear where they are coming from, even if it's a bit verbose.

Docs for reference: https://pinia.vuejs.org/core-concepts/index.html#using-the-store

Could be swayed towards using storeToRefs if the rest of us like it.

Ah, didn't know that. Thanks for the info 👍

mike-plummer · 2022-11-30T19:48:53Z

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

+import { defineStore } from 'pinia'
+import { computed, ref } from 'vue'
+import { useRouter } from 'vue-router'
+import { RunAllSpecsDataDocument, RunAllSpecsDocument } from '../generated/graphql-test'


Should probably be pulling from generated/graphql, not graphql-test

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

mike-plummer · 2022-11-30T19:55:50Z

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

+        allSpecs.push(id)
+
+        Object.keys(directoryChildren).forEach((dir) => {
+          if (id.startsWith(dir) && id.replace(dir, '').startsWith(separator)) {


nit: Could simplify

if (id.startsWith(`${dir}${separator}`) {

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

marktnoonan · 2022-11-30T23:05:07Z

packages/frontend-shared/src/locales/en-US.json

@@ -218,7 +218,8 @@
        "content": "Record a run to see your test results in Cypress Cloud. You can then optimize your test suite, debug failing and flaky tests, and integrate with your favorite tools."
      }
    },
-    "runAllSpecs": "Run {n} spec | Run {n} specs"
+    "runSelectedSpecs": "Run {n} spec | Run {n} specs",
+    "runAllSpecs": "Run all specs"


I'd suggest we do like 9.x, and when there's a filter active, make the wording be "run filtered specs" for clarity about what to expect.

I did this here 8e12cbb

lmiller1990 · 2022-12-01T04:45:08Z

@mike-plummer @ZachJW34 @astone123 @marktnoonan I pushed a commit addressing the final comments to make sure we finish this in a timely fashion. Please check 8e12cbb

Here's a preview. I also disabled the button on the inline spec list if there are 0 specs after filtering as recommended by @mike-plummer.

simplescreenrecorder-2022-12-01_14.46.32.mp4

marktnoonan · 2022-12-01T15:03:38Z

There were a couple of failures in CI, I've pushed f263211 and 3e0a325 to update the tests. Both pass for me locally now so hopefully CI will be all good now.

mike-plummer

LGTM 🚀

marktnoonan · 2022-12-01T15:45:53Z

I've noticed one of my changes is coincidentally reverting one of Lachlan's changes, seems harmless but commented here just in case.

fix: add top-level run-all button to allow all specs to be run at once

173af0f

lmiller1990 reviewed Nov 29, 2022

View reviewed changes

ZachJW34 added 2 commits November 29, 2022 14:30

address feedback

b86fb64

Merge remote-tracking branch 'origin/develop' into zachw/top-level-ru…

9babe87

…n-all-backup

ZachJW34 marked this pull request as ready for review November 29, 2022 20:31

fix ts

8855013

lmiller1990 reviewed Nov 30, 2022

View reviewed changes

mike-plummer requested changes Nov 30, 2022

View reviewed changes

marktnoonan reviewed Nov 30, 2022

View reviewed changes

respond to feedback, add label

8e12cbb

lmiller1990 force-pushed the zachw/top-level-run-all-backup branch from b59659c to 8e12cbb Compare December 1, 2022 04:42

marktnoonan added 2 commits December 1, 2022 09:56

add missing store data for component test

f263211

use RUN_ALL_SPECS_KEY constant in test instead of string

3e0a325

mike-plummer approved these changes Dec 1, 2022

View reviewed changes

Merge branch 'develop' into zachw/top-level-run-all-backup

6a29df8

marktnoonan approved these changes Dec 1, 2022

View reviewed changes

ZachJW34 merged commit 3f0d150 into develop Dec 1, 2022

ZachJW34 deleted the zachw/top-level-run-all-backup branch December 1, 2022 18:20

cypress-bot bot mentioned this pull request Dec 1, 2022

Allow running all specs without root directory #24759

Closed

marktnoonan mentioned this pull request Jan 30, 2023

test: tweak simulated run mode and some other tests for flake #25623

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add top-level run-all button to allow all specs to be run at once #24846

fix: add top-level run-all button to allow all specs to be run at once #24846

ZachJW34 commented Nov 28, 2022

cypress-bot bot commented Nov 28, 2022

lmiller1990 Nov 29, 2022

ZachJW34 Nov 29, 2022 •

edited

Loading

lmiller1990 Nov 29, 2022

ZachJW34 Nov 29, 2022

lmiller1990 Nov 30, 2022

lmiller1990 Nov 29, 2022

ZachJW34 Nov 29, 2022 •

edited

Loading

lmiller1990 left a comment

cypress bot commented Nov 29, 2022 •

edited

Loading

lmiller1990 left a comment

lmiller1990 Nov 30, 2022

lmiller1990 Nov 30, 2022

lmiller1990 commented Nov 30, 2022 •

edited

Loading

astone123 commented Nov 30, 2022

mike-plummer left a comment

mike-plummer Nov 30, 2022

lmiller1990 Dec 1, 2022

lmiller1990 Dec 1, 2022

mike-plummer Nov 30, 2022

marktnoonan Nov 30, 2022

mike-plummer Dec 1, 2022

mike-plummer Nov 30, 2022

mike-plummer Nov 30, 2022

marktnoonan Nov 30, 2022

lmiller1990 Dec 1, 2022

lmiller1990 commented Dec 1, 2022 •

edited

Loading

marktnoonan commented Dec 1, 2022

mike-plummer left a comment

marktnoonan commented Dec 1, 2022


		const isRunMode = window.__CYPRESS_MODE__ === 'run' && window.top === window

		export const useRunAllSpecsStore = defineStore('runAllSpecs', () => {

fix: add top-level run-all button to allow all specs to be run at once #24846

fix: add top-level run-all button to allow all specs to be run at once #24846

Conversation

ZachJW34 commented Nov 28, 2022

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Nov 28, 2022

Choose a reason for hiding this comment

ZachJW34 Nov 29, 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

ZachJW34 Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

cypress bot commented Nov 29, 2022 • edited Loading

Test summary

Run details

Flakiness

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

lmiller1990 commented Nov 30, 2022 • edited Loading

astone123 commented Nov 30, 2022

mike-plummer left a 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

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

lmiller1990 commented Dec 1, 2022 • edited Loading

marktnoonan commented Dec 1, 2022

mike-plummer left a comment

Choose a reason for hiding this comment

marktnoonan commented Dec 1, 2022

ZachJW34 Nov 29, 2022 •

edited

Loading

ZachJW34 Nov 29, 2022 •

edited

Loading

cypress bot commented Nov 29, 2022 •

edited

Loading

lmiller1990 commented Nov 30, 2022 •

edited

Loading

lmiller1990 commented Dec 1, 2022 •

edited

Loading