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: don't cut off a long runUrl in console #22619

Merged
merged 17 commits into from
Jul 19, 2022
Merged

Conversation

mirobo
Copy link
Contributor

@mirobo mirobo commented Jun 30, 2022

renderTables will cut off longer urls which "renders" the url unusable

User facing changelog

After a recorded run, a long runUrl would cut off which made it unusable
Recorded Run: https://......................../run/3806241066d18ec315b3a8f… ****

Additional details

Steps to test

How has the user experience changed?

Whenever the runUrl is displayed in Gitlab CI log, the URL to the run will be clickable and it will open the according run

See this incomplete URL:
image

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)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@mirobo mirobo requested a review from a team as a code owner June 30, 2022 14:25
@mirobo mirobo requested review from ryanthemanuel and removed request for a team June 30, 2022 14:25
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 30, 2022

Thanks for taking the time to open a PR!

@mirobo mirobo changed the title don't cut off a longer runUrl fix: don't cut off a long runUrl in console Jun 30, 2022
@ryanthemanuel ryanthemanuel requested review from a team and removed request for ryanthemanuel and a team June 30, 2022 15:00
@cypress
Copy link

cypress bot commented Jun 30, 2022



Test summary

37721 0 456 0Flakiness 3


Run details

Project cypress
Status Passed
Commit 72ea7ef
Started Jul 17, 2022 12:18 PM
Ended Jul 17, 2022 12:46 PM
Duration 27:13 💡
OS Linux Debian - 10.11
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/proxy-logging.cy.ts Flakiness
1 ... > intercept log has consoleProps with intercept info
2 ... > intercept log has consoleProps with intercept info
e2e/origin/commands/assertions.cy.ts Flakiness
1 cy.origin assertions > #consoleProps > .should() and .and()

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

renderTables will cut off longer urls which "renders" the url unusable
@mirobo mirobo requested a review from a team as a code owner June 30, 2022 15:24
@CLAassistant
Copy link

CLAassistant commented Jun 30, 2022

CLA assistant check
All committers have signed the CLA.

@mjhenkes mjhenkes requested a review from lmiller1990 June 30, 2022 21:46
@lmiller1990
Copy link
Contributor

Thanks for the PR! Looks like we've still got some fails. If you go into system-tests and run SNAPSHOT_UPDATE=1 yarn test reocrd_spec it should run all the specs and update the snapshots for you.

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.

Cool, just need to get CI green - see my comment.

@mirobo
Copy link
Contributor Author

mirobo commented Jul 1, 2022

Thanks for the PR! Looks like we've still got some fails. If you go into system-tests and run SNAPSHOT_UPDATE=1 yarn test reocrd_spec it should run all the specs and update the snapshots for you.

Thanks for the hint. It was extremly hard to find some sort of hint in the CI runs/logs that would point to the error. So I just searched for the string "Recorded Run"

I have to pray that it runs on my private computer though. I got quite a few weird error messages.. probably because I don't have Linux Subsystem installed on Windows (at some point it tries to execute a bash command?). When I ran yarn or read the README I couldn't find anything specific about the prerequisites for the development setup. I just installed yarn and ran the yarn command.

And the examples of running tests are probably wrong..
starting in the directory "system-test", the command "yarn test test/record_spec.js" won't work but "yarn test record_spec.js" does at least find the file. Not quite sure if this different on Mac and Windows..

I also don't know if the tests would be run with Cypress that includes my changes (do I need to build a Cypress.exe with "binary-build"?)

@lmiller1990
Copy link
Contributor

I got quite a few weird error messages.. probably because I don't have Linux Subsystem installed on Windows (at some point it tries to execute a bash command?). When I ran yarn or read the README I couldn't find anything specific about the prerequisites for the development setup. I just installed yarn and ran the yarn command.

For linux/mac should be just yarn install. I've done development on windows with git bash too, which works okay.

And the examples of running tests are probably wrong..
starting in the directory "system-test", the command "yarn test test/record_spec.js" won't work but "yarn test record_spec.js" does at least find the file. Not quite sure if this different on Mac and Windows.

Yeah this seems incorrect - we should update the docs. The system test looks inside test w/ a regexp so just yarn test record would work.

I also don't know if the tests would be run with Cypress that includes my changes (do I need to build a Cypress.exe with "binary-build"?)

Most tests run against the source code, not the bundled binary. We do have binary tests, but running those locally (except on linux) is a little annoying, so I generally recommend people just push to CI for those.

@lmiller1990
Copy link
Contributor

Something still isn't right:

image

I think we still need the table calls. I will look.

@lmiller1990
Copy link
Contributor

I think we can simplify this, pushed a commit, let's see how it goes.

@lmiller1990
Copy link
Contributor

Ok got this working but we don't actually have a test for a really long URL. I'd like to add one. @mirobo please wait a little, or if you'd like to try and add one, that'd be great, too. I have no idea how to approach adding this, though.

If you'd like to grab my commits and at least verify it's still working as you expect, that'd be fine, too.

@mjhenkes mjhenkes requested a review from astone123 July 11, 2022 20:03
@mirobo
Copy link
Contributor Author

mirobo commented Jul 11, 2022

Thanks for your additions/fixes! I'll check for a long real-life example later..

I fixed some other issues in package.json and README.MD.. I wonder what the CI builds say to this :D

I got quite a few weird error messages.. probably because I don't have Linux Subsystem installed on Windows (at some point it tries to execute a bash command?). When I ran yarn or read the README I couldn't find anything specific about the prerequisites for the development setup. I just installed yarn and ran the yarn command.

For linux/mac should be just yarn install. I've done development on windows with git bash too, which works okay.

First I needed to install Yarn globally for Windows and I needed to add the yarn binary dir (i.e. C:\Program Files (x86)\Yarn\bin) to the PATH env variable.

Building the binary is somehow required for the system-test. When I run the a system-test:

listening on port: 1234
listening on port: 3131
📦 Found package.json for project e2e.
📦 No dependencies found, skipping dep-installer steps
spawn C:\Dev\mirobo\cypress\packages\electron\dist\Cypress\Cypress.exe ENOENT
Error: spawn C:\Dev\mirobo\cypress\packages\electron\dist\Cypress\Cypress.exe ENOENT
    at Process.ChildProcess._handle.onexit (node:internal/child_process:282:19)
    at onErrorNT (node:internal/child_process:477:16)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
      1) passes

But I got this error when running "yarn binary-build":

$ C:\Dev\mirobo\cypress\node_modules\.bin\lerna run build-prod --ignore cli --concurrency 1
/usr/bin/bash: C:Devmirobocypressnode_modules.binlerna: command not found

Then I started to fiddle around with installing Lerna via NPM globally and also via Yarn globally. I was loosing my mind, I tried stuff for over an hour, googled at least half an hour and it still failed with the same error. It was a real pain!

In the end I uninstalled Lerna globally via NPM and installed it globally via Yarn. And then suddenly it started... but the joy was not for long.

"yarn binary-build" got further and produced a Cypress binary in "C:\Dev\mirobo\cypress\packages\electron\dist". But still with the following error..
It just doesn't work out of the box.. it's a real pain.. it shouldn't be so hard to set this up on a new PC.. The prerequisites are totally ambiguous.

3:09:31 All paths are short enough (29590 checked) win32
in build folder C:\Users\root\AppData\Local\Temp\cypress-build\win32\build\win-unpacked
🔥 deploy error
Error: Command failed with ENOENT: ls -la C:\Users\root\AppData\Local\Temp\cypress-build\win32\build\win-unpacked
spawn ls ENOENT
Der Befehl "ls" ist entweder falsch geschrieben oder
konnte nicht gefunden werden.
    at notFoundError (C:\Dev\mirobo\cypress\node_modules\cross-spawn\lib\enoent.js:6:26)
    at verifyENOENT (C:\Dev\mirobo\cypress\node_modules\cross-spawn\lib\enoent.js:40:16)
    at ChildProcess.cp.emit (C:\Dev\mirobo\cypress\node_modules\cross-spawn\lib\enoent.js:27:25)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12) {
  code: 'ENOENT',
  errno: 'ENOENT',
  syscall: 'spawn ls',
  path: 'ls',
  spawnargs: [
    '-la',
    'C:\\Users\\root\\AppData\\Local\\Temp\\cypress-build\\win32\\build\\win-unpacked'
  ],
  originalMessage: 'spawn ls ENOENT',
  shortMessage: 'Command failed with ENOENT: ls -la C:\\Users\\root\\AppData\\Local\\Temp\\cypress-build\\win32\\build\\win-unpacked\n' +
    'spawn ls ENOENT',
  command: 'ls -la C:\\Users\\root\\AppData\\Local\\Temp\\cypress-build\\win32\\build\\win-unpacked',
  exitCode: undefined,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: 'Der Befehl "ls" ist entweder falsch geschrieben oder\r\n' +
    'konnte nicht gefunden werden.',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Then I try to run the record_spec.js.. not a single test completes which may have to do with the fact that Cypress 10 gets hung on my PC after some usage. Need to gather some memory dumps for this..

@lmiller1990
Copy link
Contributor

You should not need to build the binary for most system tests. Some run with the binary, but the majority do not.

Are you just doing

yarn
cd system-tests
yarn test record

This should execute the record_spec.js against the real code - NOT the binary. I can fire up windows later to verify this, you might have found a bug, but I'm really surprised since I'm sure people have developed Cypress on windows in the past.

@mirobo
Copy link
Contributor Author

mirobo commented Jul 12, 2022

You should not need to build the binary for most system tests. Some run with the binary, but the majority do not.

Are you just doing

yarn
cd system-tests
yarn test record

This should execute the record_spec.js against the real code - NOT the binary. I can fire up windows later to verify this, you might have found a bug, but I'm really surprised since I'm sure people have developed Cypress on windows in the past.

See screenshots from the run:
image

image

@lmiller1990
Copy link
Contributor

I'm sorry I couldn't look at this today, I will try to give you another review and also check the state of windows + system tests before the end of the week.

@lmiller1990
Copy link
Contributor

lmiller1990 commented Jul 15, 2022

I added a test for you: 100db79

This should be ready to ship, assuming CI passes! Thanks for the contribution.

 lib/cypress
    --record
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
                                                  lib/cypress / --record / does not truncate a really long dashboard url                                                  
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------

====================================================================================================

  (Run Starting)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:    10.3.0                                                                             │
  │ Browser:    Electron 99 (headless)                                                             │
  │ Specs:      1 found (app.cy.js)                                                                │
  │ Searched:   cypress/e2e/**/*.cy.{js,jsx,ts,tsx}                                                │
  │ Params:     Tag: false, Group: electron-smoke-tests, Parallel: false                           │
  │ Run URL:    http://dashboard.cypress.io/this-is-a-long-long-long-long-long-long-long-long-long │
  │             -long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-l │
  │             ong-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-lon │
  │             g-long-long-long-long-long-long-long-long-url                                      │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running:  app.cy.js                                                                       (1 of 1)

====================================================================================================

  (Run Finished)


       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✖                                             6ms        1        2        3        4        5 │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✖  1 of 1 failed (100%)                       6ms        1        2        3        4        5  


───────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                       
  Recorded Run: http://dashboard.cypress.io/this-is-a-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-url

      ✓ does not truncate a really long dashboard url (588ms)


  1 passing (2s)

@@ -99,6 +99,53 @@ const snapshotConsoleLogs = function (name) {
return snapshot(name, stripAnsi(args))
}

function mockEE () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just moved duplicated code to this function. The implementation was not changed.

@lmiller1990
Copy link
Contributor

Looks good. I'll get one more 👀 from the e2e team, this should be able to make it into the next release. Thanks @mirobo!

@mirobo
Copy link
Contributor Author

mirobo commented Jul 18, 2022

Looks good. I'll get one more 👀 from the e2e team, this should be able to make it into the next release. Thanks @mirobo!

Thanks! I've checked the failing tests but I really, really couldn't figure out what my changes would have to do with those failures. I guess they were present on the develop branch anyway?

@flotwig flotwig self-requested a review July 18, 2022 15:33
@astone123 astone123 merged commit 9517b60 into cypress-io:develop Jul 19, 2022
@mirobo mirobo deleted the patch-1 branch July 20, 2022 07:23
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.

5 participants