feat: implement spec list tree #18901

ImCesar · 2021-11-12T22:30:55Z

Create the tree view for the spec runner

Demo

file.tree.demo.mov

PR Tasks

Have tests been added/updated?
Has the original issue or this PR been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2021-11-12T22:30:58Z

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.

CLAassistant · 2021-11-12T22:31:01Z

All committers have signed the CLA.

cypress · 2021-11-12T22:48:46Z

Test summary

18647 • 2 • 216 • 7 • 3

Run details


Project	cypress
Status	Failed
Commit	`5edcf67`
Started	Nov 16, 2021 12:39 AM
Ended	Nov 16, 2021 12:50 AM
Duration	11:23 💡
OS	Linux Debian - 10.9
Browser	Multiple

View run in Cypress Dashboard ➡️

Failures

Run group: 5x-driver-chrome (Linux, Chrome )
	commands/assertions_spec.js	1
1	... > language chainers err > throws err when assertion contains only one language chainer: and
Run group: 2x-desktop-gui (Linux, Electron )
	settings_spec.js	1
1	Settings > file preference panel > loads preferred editor and available editors

Flakiness

	commands/net_stubbing_spec.ts	2
1	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
2	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
	cypress/proxy-logging-spec.ts	1
1	... > works with forceNetworkError

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! I tested it out, works great. I also noticed we were missing some types so I added them and pushed here: 4dd17c2. Hopefully this is okay.

lmiller1990 · 2021-11-14T23:50:04Z

packages/app/src/specs/DirectoryItem.vue

+    />
+    <i-cy-folder_x16 class="mr-8px w-16px h-16px" />
+    <template
+      v-for="(directory, idx) in directories || []"


We can avoid this check with withDefaults:

withDefaults( defineProps<{ directories: string [] }>(), directories: [] )

lmiller1990 · 2021-11-14T23:51:39Z

packages/app/src/specs/InlineSpecListRow.spec.tsx

-      name: '',
-    }
+  before(() => {
+    cy.fixture('found-specs').then((foundSpecs) => specs = foundSpecs)


I think you need to do return cy.fixture. In mocha, if return a promise from before, it will wait for the promise to resolve. As it stands, I think this would be race condition.

It's never necessary to return commands or manually await them. Cypress will always wait for the queue to finish before proceeding from the hook. Commands are promise-like, but not true promises, and even .then is a command.

Cool, good to know, thanks @chrisbreiding

lmiller1990 · 2021-11-14T23:53:22Z

packages/app/src/specs/InlineSpecListTree.vue

+  }
+}
+
+const rootEl: Ref<HTMLElement | undefined> = ref()


Can be more concise; I think you can just do

const rootEl: Ref<HTMLElement> = ref()

undefined is implied by the lack of an initial value

I get Type 'Ref<HTMLElement | undefined>' is not assignable to type 'Ref<HTMLElement>'. Type 'HTMLElement | undefined' is not assignable to type 'HTMLElement'. when I try this.

Oh, my bad, you don't need Ref:

const rootEl = ref<HTMLElement>()

should suffice

Edit: I changed it to this and it's working:

const rootEl = ref<HTMLUListElement>()

We can also type the specific type of element, in this case HTMLUListElement

lmiller1990 · 2021-11-14T23:54:34Z

packages/app/src/specs/InlineSpecListTree.vue

+const onRowClick = (row: UseCollapsibleTreeNode<SpecTreeNode>, idx) => {
+  selectedItem.value = idx
+  if (row.isLeaf) {
+    router.push({ path: 'runner', query: { file: row.data?.relative } })


Does this ? make sense? Can we ever push to runner when file is null? Eg, should we be doing something like:

if (row.isLeaf) { if (!file.data?.relative) { return } router.push({ path: 'runner', query: { file: row.data?.relative } }) }

That's more of a limitation as to how we typed the row. If the node is a leaf node, it's guaranteed to have a spec on the data property. I'll see if I can tweak the types a bit more so it's happy, otherwise I'll add your check.

lmiller1990 · 2021-11-15T00:02:04Z

packages/app/src/specs/InlineSpecListTree.vue

+
+const specTree = computed(() => buildSpecTree(props.specs))
+const collapsible = computed(() => useCollapsibleTree(specTree.value, { dropRoot: true }))
+const filteredTree = computed(() => collapsible.value.tree.filter(((item) => !item.hidden.value)))


It seems specTree and collapsible is only used by filteredTree. That means we create three computed properties, but really we only want one? Should this be:

const filteredTree = computed(() => { const specTree = buildSpecTree(props.specs)) const collapsible = useCollapsibleTree(specTree.value, { dropRoot: true })) collapsible.value.tree.filter(((item) => !item.hidden.value)) })

Also, I don't think buildSpecTree should return an object with a value property. It makes it looks like it's returning a ref (because of the .value property). I was expecting to see a return type of Ref - but looking at the function, it's just a regular JS object with a non-reactive property that happens to be called value, unless I'm missing something?

I tried this but it breaks the collapsing functionality. I believe it's because the computed properties referenced from collapsible are defined inside another computed property so Vue doesn't properly track the changes.

And you're right, .value is confusing, I'll change it to .name

Yeah I noticed when I tried the refactor... This computed in a computed is a bit weird, I wonder if there's a better way.

lmiller1990 · 2021-11-15T00:28:17Z

packages/frontend-shared/src/composables/useCollapsibleTree.ts

  options.expandInitially = options.expandInitially || true
-  const collapsibleTree = buildTree(tree, options)
+  const collapsibleTree = buildTree<T>(tree, options)

  const expand = (matches?) => {


I don't see anywhere in the code base that expand or reveal is called with an argument - what is matches? Is this used?

I don't see a use for it but @JessicaSachs might have an opinion, maybe for future functionality?

…pec-list-branch

Co-authored-by: Zachary Williams <[email protected]>

elevatebart · 2021-11-15T16:42:07Z

packages/app/src/specs/InlineSpecList.vue

      <template v-else>
-        <div class="text-white">
-          FileTree not implemented
-        </div>
+        <InlineSpecListTree
+          :specs="specs.map(spec => spec.node)"
+        />
      </template>


Why use a <template> here if there is only going to be one element?
Same above.

…pec-list-branch

* 10.0-release: feat: Use plugins on config files (#18798) BREAKING CHANGE: trigger major bump BREAKING CHANGE: trigger major bump fix: fix cypress/package.json crasher fix(breaking): change circle.yml to release binary fix: build-prod-ui deps before build-prod packages feat: implement spec list tree (#18901) chore: adding 10.0-release to the circle.yml build script (#18926)

* 10.0-release: feat: improve vite DX (#18937) feat: Use plugins on config files (#18798) BREAKING CHANGE: trigger major bump BREAKING CHANGE: trigger major bump fix: fix cypress/package.json crasher fix(breaking): change circle.yml to release binary fix: build-prod-ui deps before build-prod packages feat: implement spec list tree (#18901) chore: adding 10.0-release to the circle.yml build script (#18926) feat(app): remove __vite__ route and default to unified runner (#18909) fix: app layout + specs list review (#18862) feat(app): show previous versions (#18838) feat: scaffold integration files in app (#18763) feat: add footer to the settings (#18867) fix: Exit when both --e2e and --component flags are passed in (#18855)

ImCesar requested review from JessicaSachs and a team November 12, 2021 22:30

feat: create spec tree

ba5c01a

ImCesar force-pushed the feat-spec-list-branch branch from 25b912c to ba5c01a Compare November 12, 2021 22:46

improve types

4dd17c2

lmiller1990 suggested changes Nov 15, 2021

View reviewed changes

Merge remote-tracking branch 'origin/unified-desktop-gui' into feat-s…

a8b2d52

…pec-list-branch

lmiller1990 mentioned this pull request Nov 15, 2021

feat(app): add tree list to spec page #18913

Merged

feat(app): add tree list to spec page (#18913)

7df0921

Co-authored-by: Zachary Williams <[email protected]>

elevatebart reviewed Nov 15, 2021

View reviewed changes

ZachJW34 and others added 5 commits November 15, 2021 12:16

address comments

45ce8fc

update style

91978f3

better types

4e8d71d

remove unused code

2cb227c

Merge remote-tracking branch 'origin/unified-desktop-gui' into feat-s…

a5db415

…pec-list-branch

lmiller1990 changed the base branch from unified-desktop-gui to 10.0-release November 16, 2021 00:01

remove old code

5edcf67

lmiller1990 merged commit 651b91b into 10.0-release Nov 16, 2021

lmiller1990 deleted the feat-spec-list-branch branch November 16, 2021 13:18

feat: implement spec list tree #18901

feat: implement spec list tree #18901

Uh oh!

Conversation

ImCesar commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo

PR Tasks

Uh oh!

cypress-bot bot commented Nov 12, 2021

Uh oh!

CLAassistant commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cypress bot commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Failures

Flakiness

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmiller1990 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZachJW34 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ImCesar commented Nov 12, 2021 •

edited

Loading

CLAassistant commented Nov 12, 2021 •

edited

Loading

cypress bot commented Nov 12, 2021 •

edited

Loading

lmiller1990 Nov 15, 2021 •

edited

Loading

ZachJW34 Nov 15, 2021 •

edited

Loading