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

fix: do not magically set user preferences for auto-scroll #26102

Merged
merged 25 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e172e39
fix: Do not magically mutate the user preferences when auto scroll be…
AtofStryker Mar 13, 2023
9f7f205
[run ci]
AtofStryker Mar 13, 2023
0a38681
chore: add CHANGELOG entry
AtofStryker Mar 14, 2023
0ded1eb
Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…
AtofStryker Mar 14, 2023
14a0957
Update cli/CHANGELOG.md
AtofStryker Mar 14, 2023
41c9154
Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…
AtofStryker Mar 14, 2023
28c6759
chore: move bugfix to 12.8.1 in changelog
AtofStryker Mar 14, 2023
f821276
chore: remove issue 25084 from fix
AtofStryker Mar 14, 2023
aebfa73
chore: rename autoScrollingUnderUserPreference to autoScrollingUserPref
AtofStryker Mar 14, 2023
371050e
empty
AtofStryker Mar 14, 2023
74e3230
Update packages/reporter/src/lib/events.ts
AtofStryker Mar 14, 2023
19c738b
test: add cy-in-cy regression test to make sure auto scroll is not im…
AtofStryker Mar 14, 2023
d925a92
Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…
AtofStryker Mar 14, 2023
b3ac61a
chore: update changelog entry to include #26113
AtofStryker Mar 14, 2023
88da3ed
Merge branch 'develop' into spike_scroll
AtofStryker Mar 15, 2023
cc29faf
Update system-tests/projects/cypress-in-cypress/cypress/e2e/dom-conte…
AtofStryker Mar 15, 2023
9d65937
Merge branch 'develop' into spike_scroll
AtofStryker Mar 15, 2023
e7c19d9
chore: update the specs list app test since we have added a new cy-in…
AtofStryker Mar 15, 2023
fd45631
Merge branch 'develop' of github.com:cypress-io/cypress into spike_sc…
AtofStryker Mar 15, 2023
32c1b5d
Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…
AtofStryker Mar 15, 2023
cfea4b8
chore: update the specChange-subscription tests
AtofStryker Mar 15, 2023
07099fd
Merge branch 'develop' into spike_scroll
AtofStryker Mar 15, 2023
9843a91
Update CHANGELOG.md
emilyrohrbough Mar 15, 2023
a1eb8da
chore: update missed assertion update in spec change subscription
AtofStryker Mar 15, 2023
9ad1f6f
Merge branch 'spike_scroll' of github.com:cypress-io/cypress into spi…
AtofStryker Mar 15, 2023
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
8 changes: 8 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
## 12.8.1`

_Released 03/14/2023 (PENDING)_

**Bugfixes:**

- Fixed a regression in Cypress [10](https://docs.cypress.io/guides/references/changelog#10-0-0) where the reporter auto-scroll configuration inside user preferences was unintentionally being toggled off. User's must now explicitly enable/disable auto-scroll under user preferences, which is enabled by default. Fixes [#24171](https://github.com/cypress-io/cypress/issues/24171) and [#26113](https://github.com/cypress-io/cypress/issues/26113).

## 12.8.0

_Released 03/14/2023_
Expand Down
86 changes: 61 additions & 25 deletions packages/app/cypress/e2e/reporter_header.cy.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
describe('Reporter Header', () => {
beforeEach(() => {
cy.scaffoldProject('cypress-in-cypress')
cy.openProject('cypress-in-cypress')
cy.startAppServer()
cy.visitApp()
cy.contains('dom-content.spec').click()
cy.waitForSpecToFinish()
})

context('Specs Shortcut', () => {
beforeEach(() => {
cy.scaffoldProject('cypress-in-cypress')
cy.openProject('cypress-in-cypress')
cy.startAppServer()
cy.visitApp()
cy.contains('dom-content.spec').click()
cy.waitForSpecToFinish()
})

it('selects the correct spec in the Specs List', () => {
cy.get('body').type('f')

cy.get('[data-selected-spec="true"]').should('contain', 'dom-content').should('have.length', '1')
cy.get('[data-selected-spec="false"]').should('have.length', '27')
cy.get('[data-selected-spec="false"]').should('have.length', '28')
})

// TODO: Reenable as part of https://github.com/cypress-io/cypress/issues/23902
Expand All @@ -38,26 +38,62 @@ describe('Reporter Header', () => {
})

context('Testing Preferences', () => {
it('clicking the down arrow will open a panel showing Testing Preferences', () => {
cy.get('.testing-preferences-toggle').trigger('mouseover')
cy.get('.cy-tooltip').should('have.text', 'Open Testing Preferences')

cy.get('.testing-preferences').should('not.exist')
cy.get('.testing-preferences-toggle').should('not.have.class', 'open').click()
cy.get('.testing-preferences-toggle').should('have.class', 'open')
cy.get('.testing-preferences').should('be.visible')
cy.get('.testing-preferences-toggle').click()
cy.get('.testing-preferences-toggle').should('not.have.class', 'open')
cy.get('.testing-preferences').should('not.exist')
const switchSelector = '[data-cy=auto-scroll-switch]'

context('preferences menu', () => {
beforeEach(() => {
cy.scaffoldProject('cypress-in-cypress')
cy.openProject('cypress-in-cypress')
cy.startAppServer()
cy.visitApp()
cy.contains('dom-content.spec').click()
cy.waitForSpecToFinish()
})

it('clicking the down arrow will open a panel showing Testing Preferences', () => {
cy.get('.testing-preferences-toggle').trigger('mouseover')
cy.get('.cy-tooltip').should('have.text', 'Open Testing Preferences')

cy.get('.testing-preferences').should('not.exist')
cy.get('.testing-preferences-toggle').should('not.have.class', 'open').click()
cy.get('.testing-preferences-toggle').should('have.class', 'open')
cy.get('.testing-preferences').should('be.visible')
cy.get('.testing-preferences-toggle').click()
cy.get('.testing-preferences-toggle').should('not.have.class', 'open')
cy.get('.testing-preferences').should('not.exist')
})

it('will show a toggle beside the auto-scrolling option', () => {
cy.get('.testing-preferences-toggle').click()
cy.get(switchSelector).invoke('attr', 'aria-checked').should('eq', 'true')
cy.get(switchSelector).click()
cy.get(switchSelector).invoke('attr', 'aria-checked').should('eq', 'false')
})
})

it('will show a toggle beside the auto-scrolling option', () => {
const switchSelector = '[data-cy=auto-scroll-switch]'
it('does NOT toggle off the user preferences auto-scroll if auto-scroll is temporarily disabled', () => {
cy.scaffoldProject('cypress-in-cypress')
cy.openProject('cypress-in-cypress')
cy.startAppServer()
cy.visitApp()
cy.contains('dom-content-scrollable-commands.spec').click()

// wait for the test to scroll all the way to the bottom
cy.get(':contains("checks for list items to exist - iteration #25") + :contains("passed")', {
timeout: 20000,
}).should('exist')

// then, use the runnable container to fire the scroll events that previously would override and sync the preference config
cy.get('[data-cy="reporter-panel"] .reporter > header + .container').its('0').then(($runnableContainer) => {
// scroll the container to the top.
// fire multiple scroll events so our scroller component believes the scroll came from an actual user.
[...Array(10).keys()].forEach(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if it's work a comment linking to https://github.com/cypress-io/cypress/blob/develop/packages/reporter/src/lib/scroller.ts#L37 to explain why this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would think so. My only concern is if the file changed and the reference became out of data

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah and thinking about it, if you are doing something that breaks this test, you've almost certainly been in that exact function, probably no need for comment.

$runnableContainer.dispatchEvent(new CustomEvent('scroll'))
})
})

cy.get('.testing-preferences-toggle').click()
cy.get(switchSelector).invoke('attr', 'aria-checked').should('eq', 'true')
cy.get(switchSelector).click()
cy.get(switchSelector).invoke('attr', 'aria-checked').should('eq', 'false')
})
})
})
2 changes: 1 addition & 1 deletion packages/reporter/cypress/e2e/unit/events.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ describe('events', () => {
})

it('emits save:state on save:state', () => {
appState.autoScrollingEnabled = false
appState.autoScrollingUserPref = false
appState.isSpecsListOpen = true
events.emit('save:state')
expect(runner.emit).to.have.been.calledWith('save:state', {
Expand Down
22 changes: 22 additions & 0 deletions packages/reporter/src/lib/app-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const defaults: DefaultAppState = {
}

class AppState {
@observable autoScrollingUserPref = true
@observable autoScrollingEnabled = true
@observable isSpecsListOpen = false
@observable isPaused = defaults.isPaused
Expand Down Expand Up @@ -69,6 +70,14 @@ class AppState {
this.setAutoScrolling(!this.autoScrollingEnabled)
}

/**
* Toggles the auto-scrolling user preference to true|false. This method should only be called from the
* preferences menu itself.
*/
toggleAutoScrollingUserPref () {
this.setAutoScrollingUserPref(!this.autoScrollingUserPref)
}

toggleSpecList () {
this.isSpecsListOpen = !this.isSpecsListOpen
}
Expand All @@ -88,6 +97,19 @@ class AppState {
}
}

/**
* Sets the auto scroll user preference to true|false.
* When this preference is set, it overrides any temporary auto scrolling behaviors that may be in effect.
* @param {boolean | null | undefined} isEnabled - whether or not auto scroll should be enabled or disabled.
* If not a boolean, this method is a no-op.
*/
setAutoScrollingUserPref (isEnabled?: boolean | null) {
if (isEnabled != null) {
this.autoScrollingUserPref = isEnabled
this.setAutoScrolling(isEnabled)
}
}

setStudioActive (studioActive: boolean) {
this.studioActive = studioActive
}
Expand Down
3 changes: 2 additions & 1 deletion packages/reporter/src/lib/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ const events: Events = {

localBus.on('save:state', () => {
runner.emit('save:state', {
autoScrollingEnabled: appState.autoScrollingEnabled,
// the "autoScrollingEnabled" key in `savedState` stores to the preference value itself, it is not the same as the "autoScrollingEnabled" variable stored in application state, which can be temporarily deactivated
autoScrollingEnabled: appState.autoScrollingUserPref,
isSpecsListOpen: appState.isSpecsListOpen,
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/reporter/src/lib/shortcuts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Shortcuts {
case 'n': events.emit('next')
break
case 'a': action('set:scrolling', () => {
appState.setAutoScrolling(!appState.autoScrollingEnabled)
appState.toggleAutoScrollingUserPref()
events.emit('save:state')
})()

Expand Down
3 changes: 2 additions & 1 deletion packages/reporter/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ class Reporter extends Component<SingleReporterProps> {
}

action('set:scrolling', () => {
appState.setAutoScrolling(autoScrollingEnabled)
// set the initial enablement of auto scroll configured inside the user preferences when the app is loaded
appState.setAutoScrollingUserPref(autoScrollingEnabled)
})()

action('set:specs:list', () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/reporter/src/preferences/testing-preferences.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ const TestingPreferences = observer(({
events = defaultEvents,
appState,
}: Props) => {
const toggleAutoScrolling = () => {
appState.toggleAutoScrolling()
const toggleAutoScrollingUserPref = () => {
appState.toggleAutoScrollingUserPref()
events.emit('save:state')
}

Expand All @@ -31,8 +31,8 @@ const TestingPreferences = observer(({
Auto-scrolling
<Switch
data-cy="auto-scroll-switch"
value={appState.autoScrollingEnabled}
onUpdate={action('toggle:auto:scrolling', toggleAutoScrolling)}
value={appState.autoScrollingUserPref}
onUpdate={action('toggle:auto:scrolling', toggleAutoScrollingUserPref)}
/>
</div>
<div>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
describe('Dom Content - Scrollable Assertions', () => {
beforeEach(() => {
cy.visit('cypress/e2e/dom-content.html')
})

// create enough commands in the command log to enable scrolling
;[...Array(25).keys()].forEach((idx) => {
it(`checks for list items to exist - iteration #${idx + 1}`, () => {
cy.get('li').contains('Item 1').should('exist')
cy.get('li').contains('Item 2').should('exist')
cy.get('li').contains('Item 3').should('exist')
})
})

// allow the cy-in-cy test to perform user interaction during this long test
it('waits for an arbitrary amount of time', () => {
cy.wait((50000))
})
})