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

chore: Open in IDE button for block and hook definitions in Runner UI #30859

Merged
merged 30 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
237c825
init vitest unit test harness for @packages/driver
cacieprins Jan 10, 2025
6106400
unit tests with stack examples
cacieprins Jan 10, 2025
877d8b2
drop lines until __cypress/tests rather than just __cypress
cacieprins Jan 10, 2025
c92a883
Merge branch 'develop' into cacie/fix-hook-test-stack-analysis
cacieprins Jan 10, 2025
84519d6
changelog
cacieprins Jan 10, 2025
d99c450
changelog
cacieprins Jan 10, 2025
f137526
remove vitest globals (unnecessary) from driver tsconfig
cacieprins Jan 10, 2025
6cedebc
bump the number of junit reports expected from the unit tests job
cacieprins Jan 10, 2025
c6c9d28
fix ts-check error in scroll.ts
cacieprins Jan 10, 2025
edd10e7
fix type definition for getInvocationDetails
cacieprins Jan 10, 2025
21a757e
rm packageManager key on package.json
cacieprins Jan 10, 2025
921d67f
remove junit reporter, as script that verifies result does not recogn…
cacieprins Jan 10, 2025
f406080
change @ts-expect-error to @ts-ignore for .scroll, as this ts check i…
cacieprins Jan 10, 2025
da40a47
set expected mocha result back to 19
cacieprins Jan 10, 2025
e06a2c4
add ct style stacks for cy in cy ct tests
cacieprins Jan 10, 2025
03c0865
Merge branch 'develop' into cacie/fix-hook-test-stack-analysis
cacieprins Jan 13, 2025
6d580b0
re-enable junit reporter, update mocha result verification to be more…
cacieprins Jan 13, 2025
b3018b4
persist binaries for this branch
cacieprins Jan 13, 2025
6c2ded8
expect 20 junit reports again
cacieprins Jan 13, 2025
30d0f45
fix mocha v vitest verification?
cacieprins Jan 13, 2025
cc42d8c
add binary system test to verify correct file paths for codepoints in…
cacieprins Jan 13, 2025
475fbfa
fix invocation details system test filename
cacieprins Jan 13, 2025
d0d7993
add required config for binary tests
cacieprins Jan 13, 2025
bce0e55
build on darwin to fix binary system tests
cacieprins Jan 13, 2025
0c38ce9
build linux-arm64 for this branch
cacieprins Jan 13, 2025
4996e11
simplify binary test
cacieprins Jan 13, 2025
2234703
build windows binary
cacieprins Jan 13, 2025
15ed6aa
rm binary system test
cacieprins Jan 14, 2025
b5490b3
Merge branch 'develop' into cacie/fix-hook-test-stack-analysis
cacieprins Jan 14, 2025
f70871b
Update cli/CHANGELOG.md
cacieprins Jan 14, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/workflows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1678,7 +1678,7 @@ jobs:
# run type checking for each individual package
- run: yarn lerna run types
- verify-mocha-results:
expectedResultCount: 19
expectedResultCount: 20
- store_test_results:
path: /tmp/cypress
# CLI tests generate HTML files with sample CLI command output
Expand Down
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ in this [GitHub issue](https://github.com/cypress-io/cypress/issues/30447). Addr
- Fixed a visibility issue for elements with `textContent` but without a width or height. Fixed in [#29688](https://github.com/cypress-io/cypress/pull/29688). Fixes [#29687](https://github.com/cypress-io/cypress/issues/29687).
- Elements whose parent elements has `overflow: clip` and no height/width will now correctly show as hidden. Fixed in [#29778](https://github.com/cypress-io/cypress/pull/29778). Fixes [#23852](https://github.com/cypress-io/cypress/issues/23852).
- The CSS pseudo-class `:dir()` is now supported when testing in Electron. Addresses [#29766](https://github.com/cypress-io/cypress/issues/29766).
- The Open in IDE button next to blocks and hooks in the runner UI will now properly open the IDE. This bug was introduced in a prerelease version of Cypress 14.0.0. Fixed in [#30859](https://github.com/cypress-io/cypress/pull/30859). Fixes [#30847](https://github.com/cypress-io/cypress/issues/30847).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this deserves an entry in the changelog since it was never released. I would just mark this as a chore without a changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in f70871b

cacieprins marked this conversation as resolved.
Show resolved Hide resolved

**Misc:**

Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"stop-only": "npx stop-only --skip .cy,.publish,.projects,node_modules,dist,dist-test,fixtures,lib,bower_components,src,__snapshots__ --exclude cypress-tests.ts,*only.cy.js",
"stop-only-all": "yarn stop-only --folder packages",
"pretest": "yarn ensure-deps",
"test": "yarn lerna exec yarn test --scope=cypress --scope=@packages/{config,data-context,electron,errors,extension,https-proxy,launcher,net-stubbing,network,packherd-require,proxy,rewriter,scaffold-config,socket,v8-snapshot-require,telemetry} --scope=@tooling/{electron-mksnapshot,v8-snapshot}",
"test": "yarn lerna exec yarn test --scope=cypress --scope=@packages/{config,data-context,driver,electron,errors,extension,https-proxy,launcher,net-stubbing,network,packherd-require,proxy,rewriter,scaffold-config,socket,v8-snapshot-require,telemetry} --scope=@tooling/{electron-mksnapshot,v8-snapshot}",
"test-debug": "lerna exec yarn test-debug --ignore=@packages/{driver,root,static,web-config}",
"test-integration": "lerna exec yarn test-integration --ignore=@packages/{driver,root,static,web-config}",
"test-mocha": "mocha --reporter spec scripts/spec.js",
Expand Down Expand Up @@ -278,5 +278,6 @@
"devtools-protocol": "0.0.1346313",
"sharp": "0.29.3",
"vue-template-compiler": "2.6.12"
}
},
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
}
6 changes: 5 additions & 1 deletion packages/driver/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
"cypress:run:inject-document-domain": "node ../../scripts/cypress run --config-file ./cypress.config-injectDocumentDomain.ts",
"postinstall": "patch-package",
"lint": "eslint --ext .js,.jsx,.ts,.tsx,.json, .",
"start": "node -e 'console.log(require(`chalk`).red(`\nError:\n\tRunning \\`yarn start\\` is no longer needed for driver/cypress tests.\n\tWe now automatically spawn the server in e2e.setupNodeEvents config.\n\tChanges to the server will be watched and reloaded automatically.`))'"
"start": "node -e 'console.log(require(`chalk`).red(`\nError:\n\tRunning \\`yarn start\\` is no longer needed for driver/cypress tests.\n\tWe now automatically spawn the server in e2e.setupNodeEvents config.\n\tChanges to the server will be watched and reloaded automatically.`))'",
"test": "vitest run",
"test:watch": "vitest watch"
},
"dependencies": {},
"devDependencies": {
Expand Down Expand Up @@ -58,6 +60,7 @@
"jimp": "0.22.12",
"jquery": "3.7.1",
"js-cookie": "3.0.5",
"jsdom": "^26.0.0",
"json-stable-stringify": "1.0.1",
"lodash": "^4.17.21",
"md5": "2.3.0",
Expand All @@ -81,6 +84,7 @@
"url-parse": "1.5.10",
"vanilla-text-mask": "5.1.1",
"vite": "5.2.11",
"vitest": "^2.1.8",
"webpack": "^5.88.2",
"zone.js": "0.9.0"
},
Expand Down
1 change: 1 addition & 0 deletions packages/driver/src/cy/commands/actions/scroll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ export default (Commands, Cypress, cy, state) => {
const scrollIntoView = () => {
return new Promise((resolve, reject) => {
// scroll our axes
// @ts-expect-error - scrollTo does not define a 'done()' key on its config object.
Copy link
Contributor Author

@cacieprins cacieprins Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure of why this was newly causing a ts-check failure in this branch.

The config to jquery.scrollTo doesn't accept a done key. We should consider using the onAfter key, as there may be bugs with the existing promise resolution.

return $(options.$parent).scrollTo(options.$el, {
axis: options.axis,
easing: options.easing,
Expand Down
20 changes: 16 additions & 4 deletions packages/driver/src/cypress/stack_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,17 @@ const stackWithUserInvocationStackSpliced = (err, userInvocationStack): StackAnd
}
}

type InvocationDetails = MessageLineDetail | {}
type InvocationDetails = {
absoluteFile?: string
column?: number
line?: number
originalFile?: string
relativeFile?: string
stack: string
}

const getInvocationDetails = (specWindow, config) => {
// used to determine codeframes for hook/test/etc definitions rather than command invocations
const getInvocationDetails = (specWindow, config): InvocationDetails | undefined => {
if (specWindow.Error) {
let stack = (new specWindow.Error()).stack

Expand All @@ -120,10 +128,14 @@ const getInvocationDetails = (specWindow, config) => {
// firefox and chrome throw stacks that include lines from cypress
// So we drop the lines until we get to the spec stackframe (includes __cypress)
if (specWindow.Cypress) {
stack = stackWithLinesDroppedFromMarker(stack, '__cypress', true)
// The stack includes frames internal to cypress, after the spec stackframe. In order
// to determine the invocation details, the stack needs to be parsed and trimmed.

// in Chrome and Firefox, the spec stackframe includes the pattern, '__cypress/tests'.
stack = stackWithLinesDroppedFromMarker(stack, '__cypress/tests', true)
}

const details: InvocationDetails = getSourceDetailsForFirstLine(stack, config('projectRoot')) || {};
const details: Omit<InvocationDetails, 'stack'> = getSourceDetailsForFirstLine(stack, config('projectRoot')) || {};

(details as any).stack = stack

Expand Down

Large diffs are not rendered by default.

68 changes: 68 additions & 0 deletions packages/driver/test/unit/cypress/stack_utils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @vitest-environment jsdom
*/
import { vi, describe, it, expect, beforeEach } from 'vitest'

import source_map_utils from '../../../src/cypress/source_map_utils'
import stack_utils from '../../../src/cypress/stack_utils'
import stackFrameFixture from './__fixtures__/spec_stackframes.json'

vi.mock('../../../src/cypress/source_map_utils', () => {
Copy link
Contributor Author

@cacieprins cacieprins Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could have been left unmocked, with the return value of getInvocationDetails asserted upon, however source_map_utils imports a .wasm dependency which vitest could not handle (even with vite-plugin-wasm included). It was more expedient to mock out the entire module.

return {
default: {
getSourcePosition: vi.fn(),
},
}
})

describe('stack_utils', () => {
beforeEach(() => {
// @ts-expect-error
global.Cypress = {
config: vi.fn(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results of this function call are inconsequential to the logic under test, but the lack of a global Cypress definition was throwing runtime exceptions.

}

vi.resetAllMocks()
})

describe('getInvocationDetails', () => {
const { line, column, scenarios } = stackFrameFixture

const projectRoot = '/foo/bar'

let stack: string

class MockError {
get stack () {
return stack
}
}
const config = () => projectRoot

for (const scenario of scenarios) {
const { browser, build, specFrame, stack: scenarioStack } = scenario

describe(`${browser}:${build}`, () => {
beforeEach(() => {
stack = scenarioStack
})

it('calls getSourcePosition with the correct file, line, and column', () => {
stack_utils.getInvocationDetails(
{ Error: MockError, Cypress: {} },
config,
)

// getSourcePosition is not called directly from getInvocationDetails, but via:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indirect call of the spied upon function does introduce brittleness to this test. It will not pass if the underlying implementation is changed. We should consider refactoring stack analysis, especially for user invocation codepoints, to standard patterns to ease testing flexibility.

// - getSourceDetailsForFirstLine
// - getSourceDetailsForLine
expect(source_map_utils.getSourcePosition).toHaveBeenCalledWith(specFrame, expect.objectContaining({
column,
line,
file: specFrame,
}))
})
})
}
})
})
6 changes: 4 additions & 2 deletions packages/driver/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
"noEmit": true,
"outDir": "dist",
"noErrorTruncation": true,
"types": []
"types": [],
"resolveJsonModule": true
},
"exclude": [
"dist"
"dist",
"test"
]
}
13 changes: 13 additions & 0 deletions packages/driver/vitest.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { defineConfig } from 'vitest/config'

export default defineConfig({
test: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vitest docs recommend defining a test key in the main vite.config.mjs file. However, attempting to run vitest with that configuration method resulted in runtime exceptions regarding require.resolve. Since unit tests should mock out dependencies, the aliasing of various modules there is fairly unnecessary, so far.

include: ['test/unit/**/*.spec.ts'],
environment: 'jsdom',
exclude: ['**/__fixtures__/**/*'],
reporters: [
'default',
['junit', { suiteName: 'Driver Unit Tests', outputFile: '/tmp/cypress/junit/driver-test-results.xml' }],
],
},
})
Loading
Loading