feat: Show options with Collapsible

[sainthkh]

• Closes command args are not displayed in the Command Log when options also passed into command #485
• Closes cy commands with options object should not display as Object{N} in the Command Log #678

User facing changelog

Show command options with react-inspector.

Additional details

Why was this change necessary?

Sometimes, user-defined command options are unclear like Object{N} or even ignored on reporter.

What is affected by this change?

It shows the values of options on reporter.

Any implementation details to explain?

N/A

How has the user experience changed?

Before:

After:

PR Tasks

• Have tests been added/updated?
• Has the original issue been tagged with a release in ZenHub?
• [N/A] Has a PR for user-facing changes been opened in cypress-documentation?
• Have API changes been updated in the type definitions?
• [N/A] Have new configuration options been added to the cypress.schema.json?

[cypress-bot]

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.

[jennifer-shehane]

Ran the tests in https://github.com/cypress-io/cypress-demo-ui/blob/master/cypress/integration/ui-states.js#L240 (Basically the same tests that are in the log_options_spec.js file)

Visual changes

I like the options being more clearly defined, in this case with the use of an arrow icon.

Before

After

Performance changes

This seems to be slowing down the rendering/painting of the commands when there are options passed, so that a test will run longer after this change.

I'm not sure if there's something we could handle better here.

Before

After

[sainthkh]

uncaught errors logs error on page load when new page has uncaught exception - logs error on page load when new page has uncaught exception passes on local Ubuntu 20.04. But it failed again here.

[sainthkh]

Flaky failure.

[panzarino]

Looks good to me assuming performance issues pointed out by Jennifer are fixed / improved.

[jennifer-shehane]

@sainthkh The visual changes look good and we're ok to move forward with them. Have a couple questions left.

• Has there been any changes made to address the performance concerns?
• How does this perform during cypress run. Since this isn't interact-able, so couldn't be expanded - is there any difference in the behavior? There probably should be some thought for this.

[sainthkh]

• With cypress run, options tree is expanded by default.
• Nothing has been done for performance yet. But the solution is a bit hard. I thought of lazy-calling react-inspector components. It might solve the problem of cypress open, but it cannot with cypress run because it's expanded by default. Maybe we need to give up react-inspector and create the simplified version of it.

[chrisbreiding]

Nothing has been done for performance yet. But the solution is a bit hard. I thought of lazy-calling react-inspector components. It might solve the problem of cypress open, but it cannot with cypress run because it's expanded by default. Maybe we need to give up react-inspector and create the simplified version of it.

I agree we might be best off doing this without react-inspector. I don't think having nested collapsing is that useful for this case, there aren't even that many commands with options that require it anyway, and it's probably adding a lot of performance overhead. What do you think about using our own <Collapsible> component instead and have the options displayed as a string in a <pre>?

[jennifer-shehane]

@chrisbreiding The expand is mostly helpful for things that could have larger options (basically request, screenshot, setCookie, route, etc)

@sainthkh I think I would prefer it kept looking like an object, to more closely reflect the API. Maybe an inline object if one option:

get     input
.type   Acme Company
        { force: true }

And expanded object if more than one option? Or maybe react-inspector could be used only when more than one option? So you can expand only then.

get     input
.type   Acme Company
        { 
          force: true,
          delay: 10
        }

[chrisbreiding]

The expand is mostly helpful for things that could have larger options (basically request, screenshot, setCookie, route, etc)

Sorry I wasn't clear. I was referring to options objects that have further objects or arrays nested inside of them. I agree that the root options object should be collapsed/expanded, but I don't see much benefit in collapsing nested objects. For example, if you did cy.screenshot({ log: true, blackout: ['.foo']), it might appear like this:

.screenshot 
  ▶️ { log: true }

Like this after you've expanded it:

.screenshot 
  🔽 { 
    log: true
    blackout: ▶️ [] 
  }

And then this after you clicked the blackout arrow:

.screenshot 
  🔽 {
    log: true
    blackout: 🔽 [
      '.foo'
	] 
  }

But I think it's fine to jump straight to the fully expanded version. This obviates the need for react-inspector and should make rendering more performant.

[sainthkh]

I fixed the design.

Single line

A: With <pre>

B: No <pre>

I think B looks better than A. Or are there any other suggestions?

Collapsible

Show collapsible when an object has 2+ properties or the type of its only property is not an object/array.

Future Plan

If the feature and design are right, I'll:

• expand the collapsible by default in run mode.
• write and fix tests.

[chrisbreiding]

Definitely want to go with B. My suggestion for using pre was so that the dom would be simple but have the whitespace formatting of a pre tag. I didn't mean to suggest that we should use the pre CSS styling we have in place. Using CSS white-space: pre would work also and wouldn't inherit the pre CSS styling.

[sainthkh]

Rebased and resolved conflicts.

[jennifer-shehane]

Part of why this was postponed for review is we were waiting to write some performance tests for the reporter - which has been done and exist here: https://github.com/cypress-io/cypress/blob/issue-6783/packages/driver/cypress/integration/e2e/performance_spec.ts

So, assuming that these tests test what we want and that these changes don't impact the performance - we should be able to pick this back up when we have some time.

[sainthkh]

I fixed this because it's left unresolved for a long time. The #8805 is WIP and the code will change as the things go on. I'll come back when it's done.

[jennifer-shehane]

@sainthkh Yah, we're taking a break from #8805 because the current implementation did not appear to resolve the original performance issues - based on the performance test metrics.

[badeball]

Yah, we're taking a break from #8805 because the current implementation did not appear to resolve the original performance issues - based on the performance test metrics.

That's very sad to hear. I tried the pre-release builds and I too did not experience any performance improvement in CI. Are you actively working on performance improvement at the moment? I see several issues regarding this has been closed recently, ref. #2579 and #4638.

I'm currently working on a test suite consisting of 80 specs, each which takes ~6 seconds to load. This in itself adds 8 minutes in accumulated execution time. Furthermore, every test logs around 100-200 commands, which seems to further suppress performance. At least it decimates observed / experienced performance in interactive mode.

I'm worried that Cypress doesn't take performance into account in the way I hoped and that it has become a tool which I will no longer consider.

[jennifer-shehane]

@badeball Unfortunately we're not looking into the performance issues atm.

[sainthkh]

Closing because things have changed a lot.