-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
test: final ct-audit tests and component tweaks #19948
Changes from 29 commits
0fb7178
59ac5ef
a9d69c5
d6d7248
6a7684e
2805b34
9cdc062
058d92c
7b45a10
7b33872
99fa6b6
3c61c47
8734f76
f1f9518
822596f
69a9ddd
b0450d6
4ae677f
ce0f9c6
ff36c01
c19ce91
e5eb223
619f616
b4d9f23
50e8cb8
50836bf
cc50696
78cc375
38a99a3
e567bd8
8862396
7d55755
6765eb8
422844f
3ecd0dd
2e9359d
317d759
bd5ebaa
cb9404b
d8bfbd4
db89bcc
e583107
556e0e2
9a5d20f
4135fe1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,8 @@ | |
@close="showSpecPatternModal = false" | ||
/> | ||
|
||
<template | ||
v-if="specs.length" | ||
<div | ||
v-show="specs.length" | ||
> | ||
<div | ||
class="grid grid-cols-2 children:font-medium children:text-gray-800" | ||
|
@@ -41,13 +41,17 @@ | |
> | ||
<SpecsListRowItem | ||
v-for="row in list" | ||
:id="getIdIfDirectory(row)" | ||
:key="row.index" | ||
> | ||
<template #file> | ||
<RouterLink | ||
v-if="row.data.isLeaf && row.data" | ||
:key="row.data.data?.absolute" | ||
class="focus:outline-transparent" | ||
:to="{ path: '/specs/runner', query: { file: row.data.data?.relative } }" | ||
@click.meta.prevent="handleCtrlClick" | ||
@click.ctrl.prevent="handleCtrlClick" | ||
> | ||
<SpecItem | ||
:file-name="row.data.data?.fileName || row.data.name" | ||
|
@@ -64,6 +68,7 @@ | |
:depth="row.data.depth - 2" | ||
:style="{ paddingLeft: `${((row.data.depth - 2) * 10) + 16}px` }" | ||
:indexes="getDirIndexes(row.data)" | ||
:aria-controls="getIdIfDirectory(row)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a general Vue question, is there a way to execute arbitrary code from within a template loop? Say, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, you'd have a |
||
@click="row.data.toggle" | ||
/> | ||
</template> | ||
|
@@ -77,9 +82,9 @@ | |
</SpecsListRowItem> | ||
</div> | ||
</div> | ||
</template> | ||
</div> | ||
<NoResults | ||
v-else | ||
v-show="!specs.length" | ||
:search="search" | ||
:message="t('specPage.noResultsMessage')" | ||
class="mt-56px" | ||
|
@@ -175,6 +180,19 @@ const { containerProps, list, wrapperProps, scrollTo } = useVirtualList(treeSpec | |
// If you are scrolled down the virtual list and list changes, | ||
// reset scroll position to top of list | ||
watch(() => treeSpecList.value, () => scrollTo(0)) | ||
|
||
function handleCtrlClick () { | ||
// noop intended to reduce the chances of opening tests multiple tabs | ||
// which is not a supported state in Cypress | ||
} | ||
|
||
function getIdIfDirectory (row) { | ||
if (row.data.isLeaf && row.data) { | ||
return undefined | ||
} | ||
|
||
return `speclist-${row.data.data.relative.replace(row.data.data.baseName, '')}` | ||
} | ||
</script> | ||
|
||
<style scoped> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
/* eslint-disable padding-line-between-statements */ | ||
// created by autobarrel, do not modify directly | ||
|
||
export * from './autoRename' | ||
export * from './regexps' | ||
export * from './shouldShowSteps' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -421,6 +421,8 @@ function validateExternalLink (subject, options: ValidateExternalLinkOptions | s | |
}) | ||
} | ||
|
||
Cypress.on('uncaught:exception', (err) => !err.message.includes('ResizeObserver loop limit exceeded')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is here because in CI there were 16 tests failing due to the error. The error itself is safe to ignore in that it doesn't represent something that blows up the UI and it's debatable that it should be an error instead of a warning. I'm open to suggestions for handling this differently though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This exception has been a pain in the butt forever. |
||
|
||
Cypress.Commands.add('scaffoldProject', scaffoldProject) | ||
Cypress.Commands.add('addProject', addProject) | ||
Cypress.Commands.add('openGlobalMode', openGlobalMode) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can a conditional with
v-if
be stable in the first place (what does stable mean, exactly?)If we need a DOM node that will never change, we could use the
v-once
directive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, "stable" was vague. I've moved the comment to be better placed, and updated the wording. The part with
v-if
is OK coming and going, it's the next element that shouldn't follow thev-if
rule. We would have liked to use v-show but it clashed with tailwind'sgrid
class.Maybe it's overdoing it to have this long comment in the first place, I think some "here be dragons" is useful for future developers, but there are tests now to catch regressions of the specific stuff this fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't fully understand the problem, I guess I need to pull and try it out - but that's okay, the comment should suffice. Hopefully I don't need to edit this anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specific problem that this change fixes is that if the container element is not present in the DOM with when this code runs, it doesn't update the virtualized list, so "clearing the search" from the no-results state was broken, as the old
v-if
had removed the element needed. @ZachJW34 has some ideas for making this more robust in general when we get a chance. But in this PR I just wanted to fix the bug I found when adding the coverage for that behavior, and give a bit of a here-be-dragons, because some of my first changes seemed to work great locally, but had actually broken the virtualization.cypress/packages/frontend-shared/src/composables/useVirtualList.ts
Line 84 in 317d759