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

Conversation

cacieprins
Copy link
Contributor

@cacieprins cacieprins commented Jan 10, 2025

Additional details

When a testing context is created or a hook is defined, Cypress captures the stack at that point in order to display an Open in IDE button in open mode.

With the changes to cy.origin() and the injection of document.domain, the stack is much more verbose than previously, and includes codepoints within the cypress before and after codepoints in the test file being executed.

This PR updates the stack analysis code so we can properly determine the most recent codepoint from the spec, which is used to populate the Open in IDE button.

In order to ensure correctness for this change, this PR also introduces vitest for the @packages/driver package, enabling swift and straightforward unit tests for this package in the future. Testing this via the integration test in packages/driver/cypress/e2e/cypress/stack_utils.cy.js was considered, but those tests operate more like integration tests, and preparing mocks for that situation proved more labor intensive than just using vitest.

Steps to test

Using the binary, run a test in open mode and click the "Open in IDE" button next to a test or hook block.

How has the user experience changed?

PR Tasks

@cacieprins cacieprins changed the title fix: invocation stack analysis for block invocationDetails fix: Open in IDE button for block and hook definitions in Runner UI Jan 10, 2025
@cacieprins cacieprins marked this pull request as ready for review January 10, 2025 20:04
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.

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.

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.

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.

},
{
"browser": "firefox",
"build": "dev",
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.

injectDocumentDomain stacks in firefox do not differ materially from binary or dev stacks, so that test case is omitted.

Copy link

cypress bot commented Jan 10, 2025

cypress    Run #59886

Run Properties:  status check passed Passed #59886  •  git commit f70871bba8: Update cli/CHANGELOG.md
Project cypress
Branch Review cacie/fix-hook-test-stack-analysis
Run status status check passed Passed #59886
Run duration 11m 17s
Commit git commit f70871bba8: Update cli/CHANGELOG.md
Committer Cacie Prins
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 1
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 667
View all changes introduced in this branch ↗︎
UI Coverage  38.03%
  Untested elements 132  
  Tested elements 81  
Accessibility  92.04%
  Failed rules  3 critical   7 serious   1 moderate   1 minor
  Failed elements 538  

@@ -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.

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

@cacieprins do you have the binaries available for mac? I can give it a test in linux x64 but might be good to build the binaries all around for a thorough test

@cacieprins
Copy link
Contributor Author

@AtofStryker I'll flag CI to build binaries for this. Considering adding a binary system test.

cli/CHANGELOG.md Outdated
@@ -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

@ryanthemanuel
Copy link
Collaborator

Looks good (with Matt's suggestion to remove from the Changelog)

@cacieprins cacieprins changed the title fix: Open in IDE button for block and hook definitions in Runner UI chore: Open in IDE button for block and hook definitions in Runner UI Jan 14, 2025
cli/CHANGELOG.md Outdated Show resolved Hide resolved
@cacieprins cacieprins requested a review from mschile January 14, 2025 19:16
@cacieprins cacieprins merged commit 0532f92 into develop Jan 14, 2025
112 of 120 checks passed
@cacieprins cacieprins deleted the cacie/fix-hook-test-stack-analysis branch January 14, 2025 20:33
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 16, 2025

Released in 14.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v14.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open in IDE is no longer working in Cy 14 prerelease
4 participants