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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: aut centering and height calculation #21019

Merged
merged 37 commits into from Apr 19, 2022

Conversation

marktnoonan
Copy link
Contributor

@marktnoonan marktnoonan commented Apr 11, 2022

User facing changelog

  • The AUT will be at the expected scale when the runner launches, as the reporter and specs list width used to calculate the initial scale
  • The height of the header in the AUT area will be included when calculating scale - items wrapping in the header, message, or opening the reporter will all be correctly accounted for the so AUT remains fully visible.
  • A 1 pixel vertical border that was appearing on the left hand side of screenshots created with cy.screenshot() has been fixed.

馃憜All of this brings us into parity with 9.x (though screenshots from CT in 9.x have a similar extra pixel on the left, I don't see any issues about this but still seems better to fix than to leave it wrong, especially since the extra border would now impact all screenshots including e2e).

Additional details

We were not initializing the Specs List and Reporter width values from saved preferences correctly when calculating the initial scale of the AUT. This meant we calculated the initial scale based on the default values for the reporter and specs lists sizes, even though the UI showed the correct actual size of the Reporter and Specs List. Setting those values from GQL in the store before initializing the style for the AUT fixes this.

Since I was I already in the scale math and was aware of UNIFY-1340, I added the fix for that as well, to get scaling 100% correct.

The "extra 1 px border on the left" screenshot issue doesn't have a ticket, but it caught my attention while dialing in the screenshot behavior. The screenshot behavior needed to be rechecked for this PR because changing the transform origin of the AUT and adding the left margin from 9.x would break many screenshot captures if we didn't make those changes go away during screenshotting.

Walking through the PR, these are the changes:

  • add "scales the AUT correctly" tests to cypress-in-cypress
  • start watching the height of the AUT header so that we can recalculate scale when it changes
  • clean up some imports in SpecRunnerHeaderOpenMode a bit
  • only add the 1px left hand boarder to the main-pane to avoid it showing up as an extra pixel on the left of screenshots
  • fix origin of scaling transforms, and bring over an explicit margin-left that was used in 9.x to keep things centered when scaling.
  • Reorganize how things happen in the SpecRunnerOpenMode (and run mode) script section, so that the we update the store with the computed values of specsListWidthPreferences and reporterWidthPreferences before calling useRunnerStyle() to give us the initial scale. This was the source of both the "too large for window" and "not taking enough space" category of bugs.
  • Simplified useRunnerStyle.ts a little bit, it exports some values and accepts some params that we just not need any more.
  • Replaced the old reference screenshots and fixed a typo in the folder name.
  • Almost got the screenshots into Percy, but created https://cypress-io.atlassian.net/browse/UNIFY-1566 for that instead when it seemed to have some nuance.

How has the user experience changed?

Old resizing behavior

Shows all three bugs - starting out scaled to small, header wrapping causing bottom of AUT to disappear, and starting out scaled too large, cutting off right side of AUT):

all.3.bugs.in.10.0-release.mov

New resizing behavior

Sows that all 3 bugs cannot be recreated in the same ways, full AUT iframe is always visible, and when scaled, the margins and centering are correct.

resizing.working.nicely.mov

Old vs new screenshots

No longer has that 1px border on the left.

old vs new screenshots

9.x screenshots from its versions of the spec had the same extra pixel, caused by same issue of a border hanging around:

Screen Shot 2022-04-13 at 8 43 19 AM

Testing

This PR adds percy snapshots for various sizes of the runner and confirms that we use the value from GQL for the initial scale calculation. These snapshots won't add value until #20604 is merged, since Cy-in-Cy tests do not render in Percy.

To manually test, follow the example in the videos. Resize the reporter to much larger or much smaller than the default. And refresh the page. Prior to this PR, scaling should start out wrong, after this PR scaling should start out correct.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • 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
Copy link
Contributor

cypress-bot bot commented Apr 11, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Apr 11, 2022



Test summary

17872 0 217 0Flakiness 2


Run details

Project cypress
Status Passed
Commit 15e75ea
Started Apr 18, 2022 2:29 PM
Ended Apr 18, 2022 2:44 PM
Duration 14:56 馃挕
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard 鉃★笍


Flakiness

commands/xhr.cy.js 1聽Flakiness
1 ... > can alias a route without stubbing it
create-react-app.cy.ts 1聽Flakiness
1 Working with cra-5 > should detect new spec

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

@marktnoonan marktnoonan marked this pull request as ready for review April 13, 2022 13:52
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

When I run the new test you added in open mode, the scales the AUT correctly in e2e always fails, though it passes when run in isolation. Are you seeing this?

Other than that looks good, not sure if my machine is being weird and CI is green so I'm going to 鉁旓笍

return props.gql.localSettings.preferences.reporterWidth ?? runnerUiStore.reporterWidth
})

// we must update pr before calling userRunnerStyle, to make sure that values from GQL
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment seems incomplete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dang I did not see that test failure, gonna check it out and fix that comment!

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 was able to see it fail but only once. I think though, we might have run afoul of some gql updates when trying to close the specs list so soon after the doing the last resize earlier in the test - from what I saw the reporter ended up too wide, which could be explained if closing the specs list happened to resolve before updating the width.

I adjusted the test a bit get better separation between those things. Would be good to know if you can still reproduce? Though I think it represents something no user can every be fast enough to do.

@ZachJW34
Copy link
Contributor

@marktnoonan tested the changes, tests are passing locally for me now

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Looks great! I have a ton of comments/questions, mainly around 1) how the heck some of this works and 2) maintainability (mainly around code duplication, I see this quickly becoming a point of regressions between run vs open mode).

PS I am so glad that 1px is gone, it was bothering me a lot, good to see we are in polish mode now.

packages/app/cypress/e2e/cypress-in-cypress.cy.ts Outdated Show resolved Hide resolved
packages/app/src/runner/SpecRunnerHeaderOpenMode.vue Outdated Show resolved Hide resolved
packages/app/src/runner/SpecRunnerHeaderRunMode.vue Outdated Show resolved Hide resolved
packages/app/src/runner/SpecRunnerOpenMode.vue Outdated Show resolved Hide resolved
packages/app/src/runner/SpecRunnerRunMode.vue Outdated Show resolved Hide resolved
packages/app/src/runner/SpecRunnerRunMode.vue Outdated Show resolved Hide resolved
packages/app/src/runner/useRunnerStyle.ts Show resolved Hide resolved
packages/app/src/store/aut-store.ts Show resolved Hide resolved
@tbiethman
Copy link
Contributor

The functionality works great. I'll wait for the suggested refactors to land before giving a final review.

@lmiller1990
Copy link
Contributor

@marktnoonan can you re-ping me when this is ready to re-review? Thanks.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

One last comment

packages/app/src/runner/useAutHeader.ts Show resolved Hide resolved
@marktnoonan
Copy link
Contributor Author

Ok, updates have been made for @lmiller1990's feedback, ready for you @tbiethman :)


return {
autHeaderEl,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider building a full-on AutHeader component at some point (not this PR). There's enough shared logic now that is would clean up the header containers quite a bit, even if we have to disable most of the functionality in run mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

Try as I might, I can't break the new scaling logic 馃帀 new test coverage looks good, lots of good reuse, no complaints from me

@lmiller1990
Copy link
Contributor

Let's ship it

@lmiller1990 lmiller1990 merged commit b8d23db into 10.0-release Apr 19, 2022
@lmiller1990 lmiller1990 deleted the UNIFY-1380-aut-initial-size branch April 19, 2022 10:08
tgriesser added a commit that referenced this pull request Apr 20, 2022
鈥-config

* 10.0-release:
  chore: Move component-index generation to scaffold-config package (#21090)
  fix: label text should be clickable to toggle snapshot highlight (#21122)
  feat: add next preset to webpack-dev-server-fresh (#21069)
  chore: add dev-servers as deps to server to be included in the binary (#21142)
  fix: do not highlight preExtension if selected option is renameFolder (#21121)
  fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser (#21063)
  feat(unify): launchpad header breadcrumbs and reusable tooltip component (#20648)
  test: add windows-test-kitchensink job (#20847)
  fix: aut centering and height calculation (#21019)
  chore: fix tests that fail on windows (#21055)
  fix: running a new test after already having run tests (#21087)
  fix: ct project setup framework keys for next and nuxt (#21116)
  fix: remove MountReturn from scaffolded ct support file (#21119)
  fix: do not scaffold fixtures if folder exist (#21078)
  fix: revert "fix: types for Cypress.Commands.add (#20376)" (#21104)
  chore: Update Chrome (stable) to 100.0.4896.127 and Chrome (beta) to 101.0.4951.34 (#21083)
  fix: types for Cypress.Commands.add (#20376) (#20377)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants