cypress icon indicating copy to clipboard operation
cypress copied to clipboard

fix: typo in viewport dropdown

Open FireNdIce3 opened this issue 3 years ago • 5 comments

  • Closes #23632

User facing changelog

Additional details

Steps to test

How has the user experience changed?

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?

FireNdIce3 avatar Sep 12 '22 19:09 FireNdIce3

Thanks for taking the time to open a PR!

cypress-bot[bot] avatar Sep 12 '22 19:09 cypress-bot[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 12 '22 19:09 CLAassistant

** Ignore this comment! ** commit https://github.com/cypress-io/cypress/commit/4b09dc3b781cf5bc0baef85ab3b4c4cef1872444 solves the issue.


I was unable to resolve the issue locally so I have not created a PR. I took some notes while testing / exploring and I will include them here.

Files

  1. packages/frontend-shared/src/locales/en-US.json
  2. packages/app/src/runner/SpecRunnerHeaderOpenMode.vue
  3. packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx
  4. packages/app/cypress/e2e/cypress-in-cypress-e2e.cy.ts

Origin

The issue seems to originate within the function responsible for parsing and replacing in-string placeholder values with real values. This text has string-vars like: in string var like {this} replaced.

The original (unreplaced) text segment exists in a locale source (file-1) at runner.viewportTooltip.configText

The adjacent entry of runner.viewportTooltip.infoText has its placeholder values correctly replaced at {0}, {1} and {2}


The locale object values are passed to an <i18n-t> component as the keypath prop in (file-2)

<i18n-t
  tag="p"
  keypath="runner.viewportTooltip.configText"
  class="mb-24px"
>

The test file (file-3) contains a test at line 196 that checks for the existence and visibility of the correctly replaced string:

cy.contains('Additionally, you can override this value in your cypress.config.js or via the cy.viewport() command.')
                                                               ^^^^^^^^^^^^^^^^^            ^^^^^^^^^^^^^
.should('be.visible')

There is also an example cypress test in (file-4) that tests for the adjacent config text runner.viewportTooltip.infoText but, does not test for runner.viewportTooltip.configText

pratiqdev avatar Sep 20 '22 17:09 pratiqdev

@FireNdIce3 Do you think you'll be able to add a test for this?

chrisbreiding avatar Sep 20 '22 18:09 chrisbreiding

@chrisbreiding Interestingly our Component tests for this are now failing with this fixed behavior which was testing it originally https://app.circleci.com/pipelines/github/cypress-io/cypress/43400/workflows/3eb83d3f-5f15-496c-a78c-d9e62e697373/jobs/1811601

  • https://github.com/cypress-io/cypress/blob/develop/packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx#L184
  • https://github.com/cypress-io/cypress/blob/develop/packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx#L207

Wonder why these are inconsistent 🤔

emilyrohrbough avatar Sep 20 '22 18:09 emilyrohrbough

Interestingly our Component tests for this are now failing with this fixed behavior which was testing it originally app.circleci.com/pipelines/github/cypress-io/cypress/43400/workflows/3eb83d3f-5f15-496c-a78c-d9e62e697373/jobs/1811601

[develop/packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx#L184](https://github.com/cypress-io/cypress/blob/develop/packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx?rgh-link-date=2022-09-20T18%3A31%3A16Z#L184)
[develop/packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx#L207](https://github.com/cypress-io/cypress/blob/develop/packages/app/src/runner/SpecRunnerHeaderOpenMode.cy.tsx?rgh-link-date=2022-09-20T18%3A31%3A16Z#L207)

Wonder why these are inconsistent 🤔

@emilyrohrbough Took a little digging, but I figured this out and figured out the root cause of the original bug.

The <i18n-t> component can do string interpolation in two ways. The way we were using it (and still using it in other places) relied on list interpolation, where {0}, {1}, etc rely on the order and placement of the children nested within the <i18n-t> component.

The original code looks like this (with children indexes noted):

    <i18n-t ...>
0:    <!-- disable rule to prevent trailing space from being added to <InlineCodeFragment/> -->
1:    <!-- eslint-disable-next-line vue/singleline-html-element-content-newline -->
2:    <InlineCodeFragment class="font-medium text-xs leading-5">{{ props.gql.configFile }}</InlineCodeFragment>
3:    <!-- eslint-disable-next-line vue/singleline-html-element-content-newline -->
4:    <InlineCodeFragment class="font-medium text-xs leading-5">cy.viewport()</InlineCodeFragment>
    </i18n-t>

So we used the interpolated values {2} and {4}. This works locally, in development, and in CI.

However, I'm guessing that when we build for production and produce the binary, the comments are stripped out before it does the i18n replacement, so the Vue markup ends up looking like this:

    <i18n-t ...>
0:    <InlineCodeFragment class="font-medium text-xs leading-5">{{ props.gql.configFile }}</InlineCodeFragment>
1:    <InlineCodeFragment class="font-medium text-xs leading-5">cy.viewport()</InlineCodeFragment>
    </i18n-t>

So then the interpolated values should be {0} and {1}. {2} and {4} don't even exist, so it ends up putting in empty strings (it would be nice if the build failed in that case, eh?). That's why the component tests used to pass but failed when using {0} and {1}. But {0} and {1} would have worked in the production app.

To resolve this, I changed the markup to use the other string interpolation method, slots syntax. It ensures the same values are interpolated regardless of child order or the presence/lack-of-presence of comments.

chrisbreiding avatar Sep 22 '22 18:09 chrisbreiding

@chrisbreiding whoa....nice find! I bet you are correct that prod mode was stripping these out.

emilyrohrbough avatar Sep 22 '22 18:09 emilyrohrbough