feat: output video file path on one row during run mode #23915

zachz-indeed · 2022-09-21T15:08:38Z

Closes Make video file path clickable #23913

User facing changelog

Moves video output link to it's own line in run mode to make the video output link more easily clickable in the terminal.

Additional details

Improves the user experience to allow users to directly click the recorded link inside the terminal or copy/paste from a CI terminal more easily

Steps to test

Easiest way to see changes locally is to run a single test in cypress run mode. For omitting values under system-tests in the core repo, please run the system tests and compare the snapshot.

How has the user experience changed?

Before

video-link-not-clickable.mp4

After

video-link-clickable.mp4

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?

cypress-bot · 2022-09-21T15:08:42Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

CLAassistant · 2022-09-21T15:08:50Z

All committers have signed the CLA.

mjhenkes · 2022-09-29T16:18:05Z

@zachz-indeed, This seems like a good change. Could you provide a before and after screenshot for us to compare?

zachz-indeed · 2022-10-10T17:31:40Z

(Ignore font color differences)

Before:

After:

zachz-indeed · 2022-10-11T16:42:05Z

@mjhenkes - I added a before/after screenshot of the behavior.

I'm also getting an 'Unauthorized' status in CircleCI on the build step as of this morning. Do you have any ideas on what may have caused it/how to resolve? Thanks!

nagash77 · 2022-10-28T12:49:22Z

Hello @zachz-indeed ! Thank you for opening a PR to Cypress. We have been reviewing your PR internally and have a few changes we would like before this gets merged:

Please add two spaces and a dash like all the previous lines to the lines you have added.
Change to Finished processing: X seconds (no aligned-right seconds)
Ensure it works with progress lines between ‘Started…’ and ‘Finished…’ lines and please attach screenshots of the larger context in which your changes are displayed.

zachz-indeed · 2022-11-04T18:00:34Z

Hi @nagash77 - thank you for the feedback! I have made the recommended changes. Here is a screenshot of what that looks like now:

I will work on getting the snapshots updated as well. Please let me know if there's anything else I should update or provide. This is my first open source contribution, so learning a lot as I go :) Thanks again!

lmiller1990 · 2022-12-22T00:44:00Z

Hi @zachz-indeed - I can help you get this merged. It looks like some of the snapshots are not updated or conflicted.

If you want to merge in develop and resolve the conflicts, I can handle updating the snapshots - you just need to run the system test with SNAPSHOT_UPDATE=1, but it takes a really long time, since this PR is basically updating every snapshot (hundreds).

It's probably quicker if someone who works in the repo takes care of that, so if you just want to resolve the conflicts, I can help with the snapshot updates.

AtofStryker · 2022-12-29T15:05:48Z

@lmiller1990 @zachz-indeed I should be able to take a look at this today and get the snapshots updated. Might take a while though 😅

emilyrohrbough · 2022-12-29T15:21:01Z

packages/server/lib/util/print-run.ts

@@ -508,13 +508,16 @@ export function displayVideoProcessingProgress (opts: { videoName: string, video
        table.push([
          gray('-'),
          gray('Finished processing:'),
-            `${formatPath(opts.videoName, getWidth(table, 2), 'cyan')}`,
-            gray(dur),
+          gray(dur),
        ])

        console.log(table.toString())

        console.log('')


Not sure we need a new line between the video details and the video output name.

AtofStryker · 2022-12-30T19:01:43Z

one of the issues I am running into is that the processing time may/may not vary, which leads to some flakiness within our snapshots. Though I agree that I think the process time is more useful, the lift might be larger here to actually implement within our snapshots. I think for this reason we will likely need to go back to (x second), or possibly put this behind a flag where we can toggle the actual time being printed vs omitting it. This way our snapshots do not flake, and end users get the benefit of seeing the time. How does this sound @emilyrohrbough @lmiller1990 ?

emilyrohrbough · 2022-12-30T19:04:05Z

@AtofStryker The processing time should be overridden in the snapshots for (x seconds) like you are saying. It looks like these are captured correctly in the snapshot changes. Where are you seeing this at?

NEVERMIND - I see it's in the test failures. Yeah the regex likely needs updated to correctly account for this if this isn't being process correctly.

astone123 · 2023-01-10T18:54:20Z

Snapshots are being correctly processed now. We just need to update the snapshots for all of the system tests

AtofStryker · 2023-01-11T15:11:34Z

I'm updating the snapshots now and should have this pushed up today

AtofStryker · 2023-01-13T22:02:11Z

OK I got the snapshots updated 😅 . Took a while to figure it out but everything should now be green in CI

cypress · 2023-01-13T22:04:48Z

Test summary

5033 • 0 • 75 • 0 • 7

Run details


Project	cypress
Status	Passed
Commit	`f6aee7c`
Started	Jan 17, 2023 7:36 PM
Ended	Jan 17, 2023 7:50 PM
Duration	13:35 💡
OS	Linux Debian -
Browser	Electron 106

View run in Cypress Dashboard ➡️

Flakiness

	e2e/origin/cookie_behavior.cy.ts	4
1	... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
2	... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
3	... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
4	... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
	cypress/cypress.cy.js	1
1	... > correctly returns currentRetry
This comment includes only the first 5 flaky tests. See all 7 flaky tests in the Cypress Dashboard.

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

lmiller1990 · 2023-01-17T01:37:01Z

packages/driver/cypress/e2e/e2e/security.cy.js

@@ -3,4 +3,21 @@ describe('security', () => {
    cy.visit('/fixtures/security.html')
    cy.get('div').should('not.exist')
  })
+


Was the intentionally merged here?

I think something went wrong during the rebase, it looks like some extra files in proxy and this one got added.

something is off as this branch is missing 478407e. What I might do to make this change set simpler is to create a new branch with a single commit of the changes based off develop. @zachz-indeed you would likely need to recreate the branch locally when this happens

I think we need someone to own this, I think we should just make a new branch and pick the files we need. I can do it if no-one else has bandwidth, but probably not until the first week of Feb.

I took care of it. 0843b0a is the commit with files we need with snapshots updated with the CI run in f6aee7c. This should be ready for review, which I don't feel super comfortable reviewing simply because of how many changes I committed.

I took a look, I think it looks fine. Any particular change you are concerned with? It's a lot of files, but all the changes are the same and what I'd expect.

nothing in particular. I think the two src file updates make sense and the snapshots are G2G and match what is expected

Co-authored-by: Zach <zachz@indeed.com> Co-authored-by: Bill Glesias <bglesias@gmail.com>

AtofStryker · 2023-01-17T18:00:30Z

@lmiller1990 @emilyrohrbough @zachz-indeed I got the necessary change set based on 0843b0a. This should only contain the changes to stdout, printing the run, and any snapshot updates needed

lmiller1990

One of the snapshots has some unexpected changes (maybe). The rest looks good. I think we should find out about that one snapshot, and see if that was correct.

lmiller1990 · 2023-01-18T22:08:00Z

system-tests/__snapshots__/browser_crash_handling_spec.js




-We detected that the Chrome process just crashed with code 'null' and signal 'SIGTRAP'.
+We detected that the Chromium Renderer process just crashed.


I wonder why this changed 🤔

Oh I see, I think the snapshots in the file are just in a different order now. It's just confusing now GH shows this as a diff - I guess the order than @AtofStryker ran the updates locally was different to what the previous person who updated the snapshots ran, so the order they were wrote to the file is changed.

Getting these snapshots to update correctly was a bit of a challenge 😅 . Sometimes the snapshots do update in a different order but should be the same relative to their export.

I noticed we have a conflict in browser_crash_handling_spec from the improved memory work. I am rebuilding that snapshot now to fix the conflict and will update @zachz-indeed 's branch shortly

cypress-bot · 2023-01-24T20:59:10Z

Released in 12.4.0.

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

nagash77 requested review from astone123 and mjhenkes September 27, 2022 18:09

zachz-indeed changed the title ~~ISSUE-23913: output video file path on one row~~ fix: ISSUE-23913 output video file path on one row Oct 7, 2022

zachz-indeed force-pushed the issue-23913 branch from 4075971 to 591967c Compare October 7, 2022 15:18

mjhenkes marked this pull request as ready for review October 11, 2022 17:26

nagash77 requested review from AtofStryker and warrensplayer October 12, 2022 18:04

nagash77 assigned mike-plummer Dec 6, 2022

mike-plummer assigned lmiller1990 and unassigned mike-plummer Dec 20, 2022

emilyrohrbough reviewed Dec 29, 2022

View reviewed changes

lmiller1990 mentioned this pull request Jan 17, 2023

feat(breaking): Remove whitespaces from screenshot names to make it terminal friendly #24355

Closed

4 tasks

lmiller1990 reviewed Jan 17, 2023

View reviewed changes

zachz-indeed and others added 2 commits January 17, 2023 11:20

fix: ISSUE-23913 output video file path on one row

0843b0a

Co-authored-by: Zach <zachz@indeed.com> Co-authored-by: Bill Glesias <bglesias@gmail.com>

run ci

f6aee7c

AtofStryker force-pushed the issue-23913 branch from ddafab1 to f6aee7c Compare January 17, 2023 17:59

AtofStryker requested review from emilyrohrbough and lmiller1990 and removed request for AtofStryker January 17, 2023 19:10

lmiller1990 reviewed Jan 18, 2023

View reviewed changes

lmiller1990 self-requested a review January 18, 2023 22:13

lmiller1990 approved these changes Jan 18, 2023

View reviewed changes

AtofStryker added 3 commits January 19, 2023 10:10

Merge branch 'develop' of github.com:cypress-io/cypress into issue-23913

e297239

run ci

3bbb1e9

Merge branch 'develop' into issue-23913

c57e26b

BlueWinds approved these changes Jan 19, 2023

View reviewed changes

mjhenkes approved these changes Jan 19, 2023

View reviewed changes

AtofStryker changed the title ~~fix: ISSUE-23913 output video file path on one row~~ feat: ISSUE-23913 output video file path on one row Jan 19, 2023

AtofStryker changed the title ~~feat: ISSUE-23913 output video file path on one row~~ feat: output video file path on one row during run mode Jan 19, 2023

AtofStryker requested review from AtofStryker and removed request for AtofStryker January 19, 2023 18:38

AtofStryker merged commit 05d35f6 into cypress-io:develop Jan 19, 2023

cypress-bot bot locked as resolved and limited conversation to collaborators Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: output video file path on one row during run mode #23915

feat: output video file path on one row during run mode #23915

zachz-indeed commented Sep 21, 2022 •

edited by AtofStryker

Loading

cypress-bot bot commented Sep 21, 2022

CLAassistant commented Sep 21, 2022 •

edited

Loading

mjhenkes commented Sep 29, 2022

zachz-indeed commented Oct 10, 2022

zachz-indeed commented Oct 11, 2022

nagash77 commented Oct 28, 2022

zachz-indeed commented Nov 4, 2022

lmiller1990 commented Dec 22, 2022

AtofStryker commented Dec 29, 2022

emilyrohrbough Dec 29, 2022

AtofStryker commented Dec 30, 2022

emilyrohrbough commented Dec 30, 2022 •

edited

Loading

astone123 commented Jan 10, 2023

AtofStryker commented Jan 11, 2023

AtofStryker commented Jan 13, 2023

cypress bot commented Jan 13, 2023 •

edited

Loading

lmiller1990 Jan 17, 2023

AtofStryker Jan 17, 2023

lmiller1990 Jan 18, 2023

AtofStryker Jan 18, 2023

lmiller1990 Jan 18, 2023

AtofStryker Jan 19, 2023

AtofStryker commented Jan 17, 2023

lmiller1990 left a comment

lmiller1990 Jan 18, 2023

lmiller1990 Jan 18, 2023 •

edited

Loading

AtofStryker Jan 19, 2023

cypress-bot bot commented Jan 24, 2023




		We detected that the Chrome process just crashed with code 'null' and signal 'SIGTRAP'.
		We detected that the Chromium Renderer process just crashed.

feat: output video file path on one row during run mode #23915

feat: output video file path on one row during run mode #23915

Conversation

zachz-indeed commented Sep 21, 2022 • edited by AtofStryker Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

Before

After

PR Tasks

cypress-bot bot commented Sep 21, 2022

CLAassistant commented Sep 21, 2022 • edited Loading

mjhenkes commented Sep 29, 2022

zachz-indeed commented Oct 10, 2022

zachz-indeed commented Oct 11, 2022

nagash77 commented Oct 28, 2022

zachz-indeed commented Nov 4, 2022

lmiller1990 commented Dec 22, 2022

AtofStryker commented Dec 29, 2022

Choose a reason for hiding this comment

AtofStryker commented Dec 30, 2022

emilyrohrbough commented Dec 30, 2022 • edited Loading

astone123 commented Jan 10, 2023

AtofStryker commented Jan 11, 2023

AtofStryker commented Jan 13, 2023

cypress bot commented Jan 13, 2023 • edited Loading

Test summary

Run details

Flakiness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Jan 17, 2023

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress-bot bot commented Jan 24, 2023

zachz-indeed commented Sep 21, 2022 •

edited by AtofStryker

Loading

CLAassistant commented Sep 21, 2022 •

edited

Loading

emilyrohrbough commented Dec 30, 2022 •

edited

Loading

cypress bot commented Jan 13, 2023 •

edited

Loading

lmiller1990 Jan 18, 2023 •

edited

Loading