Screenshot fixes/enhancements

[chrisbreiding]

Fixes #1766

If multiple non-named screenshots are taken in a single test, after the first one they're appended with a number, i.e. test name (1).png.

Fixes #1771

Passing foo/bar/baz as the name of spec app.spec.js will result in the screenshot being saved to screenshots/app.spec.js/foo/bar/baz.png.

Fixes #1826

By default, screenshots will be saved in a directory structure matching the spec's path. cypress/integration/foo/bar/baz_spec.js will save a screenshot as screenshots/foo/bar/baz_spec.js/test name.png.

Fixes #1923

We now append -- failed to screenshots taken automatically during failure

Also fixes #1919 and #1918

Documentation PR: cypress-io/cypress-documentation#675

closes #1881

[brian-mann]

So on a cursory review - it seems like you're doing a lot of backflips to get the specPath out of the driver and passing it around to the server.

It is almost certainly easier to handle the specPath through the server itself - so you don't have to parse out anything from the driver or URL, etc.

But I say this with a grain of salt that I have not looked into doing this, and maybe you saw that this was more difficult.

My assumption was that you didn't need to change anything on the driver end and that this could be all implemented inside of server/lib/screenshot minus the notion of the automatic failure screenshot.

[chrisbreiding]

Some of the changes regarding the spec path is refactoring, so not everything in the diff is necessarily related to the screenshot path.

We were already parsing the spec path out of the URL. It's more-or-less the source of truth for the currently selected spec.

AFAIK, the only place we hold onto the currently running spec in the server is in lib/socket and it's pretty much constrained to one function. I figured holding on to that state in the driver was a safer and potentially easier bet than adding more state to the server.

That said, I'm open to a better solution if there is one.

[brian-mann]

@chrisbreiding right. So it's currently architected that way, but with the new 3.0 spec isolation changes it should be trivial to bootstrap the spec into the driver.

That would get rid of parsing the URL entirely (you'd just get it from this state) and we could expose a property on Cypress: Cypress.spec that points to this as well.

I get that in interactive mode it's a bit trickier holding onto the open spec than say run mode, but it'll likely be cleaner keeping all the state in a single place. I can take it from here - just wanted to make sure I wasn't missing something.

[jennifer-shehane]

This will require some updates to the documentation. I opened an issue here: cypress-io/cypress-documentation#671

[brian-mann]

Also fixes #1919 and #1918

[brian-mann]

@bahmutov need your made typescript skillz for #1919 and #1918

Gonna release tomorrow

[bahmutov]

@brian-mann I have added types for Cypress.spec and Cypress.browser using the test from b68c549

also trimmed duplicate kitchensink code

[brian-mann]

I'm making a few changes here:

• a failure screenshot will be called (failed) as opposed to -- failure.
• instead of a named screenshot being placed at the root screenshotsFolder it is nested based on specName
• named screenshots will also have duplicate numbers (1) appended to them
• i moved the state out of the driver / runner and kept it entirely in the server minus takenPaths
• i'm not sanitizing the folder names as much, I'm leaving in dots . since after doing some research it should not matter and i'd rather keep it closer to what the spec was called
• the spec name now acts as a bona-fide folder instead of being part of the screenshot title.