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

Canvas elements should restore drawn state on time travel with before and after snapshots #1770

Open
benpolinsky opened this issue May 24, 2018 · 15 comments
Labels
topic: snapshot type: feature New feature that does not currently exist

Comments

@benpolinsky
Copy link

benpolinsky commented May 24, 2018

Current behavior:

When I run the "actions" tests in the provided examples, the clicks on the canvas show while the test is running, but do not show when going back through them in the command log. In fact, you can't even manually click the canvas and create a dot in the runner.

Desired behavior:

Time travel and the command log should show before and after versions of what happens when you click on a canvas.

Steps to reproduce:

Install Cypress. Run the examples. Use the command log to time travel to the Actions section and attempt to view the canvas before and after each click.

Versions

Cypress - 2.1.0
Windows 10 - 1703 Build
Chrome - 66

@benpolinsky benpolinsky changed the title Canvas Example in example_spec doesn't time travel well Canvas example in example_spec doesn't time travel well May 24, 2018
@jennifer-shehane
Copy link
Member

Can verify, this issue is happening as described.

  • Canvas is not restored to its drawn state on hover of command log.
  • Canvas cannot be interacted with while DOM snapshot is pinned.

@jennifer-shehane jennifer-shehane added the type: unexpected behavior User expected result, but got another label May 24, 2018
@jennifer-shehane jennifer-shehane changed the title Canvas example in example_spec doesn't time travel well Canvas elements should restore drawn state on time travel with before and after snapshots Feb 11, 2019
@cypress-bot cypress-bot bot added the stage: ready for work The issue is reproducible and in scope label Feb 11, 2019
@callumacrae
Copy link

Did this used to have something to replicate with? I'm experiencing the same issue (I think) so happy to provide one if it would mean we get a fix quicker :)

@RodrigoHamuy
Copy link

Hi, any updates about this? Thanks.

@fr0
Copy link

fr0 commented Nov 13, 2019

Can this be fixed? I'm testing a page that makes heavy use of the canvas and this bug is making it difficult. This issue has been open for a year and a half.

@stevenalanstark
Copy link

I would not expect this to be changed. In order to have time travel with canvas elements means that cypress would need to draw and record the canvas at every single step. This would be a very memory hungry process and would promote cypress crashes.

Even if time travel was implemented, it would not be interactive. This is simply not possible with Canvas.

@callumacrae
Copy link

@stevenalanstark how would it be any more memory hungry than recording images?

@stevenalanstark
Copy link

stevenalanstark commented Jan 9, 2020

@callumacrae Cypress is already past it's limit with memory consumption with screenshots and video recording alone, which will often crash the runner due to memory issues, but will run smoothly when in headless mode because it doesn't need to keep the history in headless mode.

note: the time travel is recording the DOM, and not taking screenshots.

At least with screenshots and video it's a 1 to 1 relationship, each test will produce one screenshot / video when the command is called

If you're wanting to apply this logic into the time travel history, it's already going to be far too much memory alone, so in short: No recording images is not even doing this. There is no time travel associated with screenshots. They do record the DOM elements, and that alone is a big ask.

So now with canvas recording you are moving into a 1 to many relationship with each test because you can have more than one canvas on a page at any given time. Let's say you have 3 canvas', and each screenshot is taking only 50kb because these are very small canvas', and you have a full suite of tests with 1000 individual steps that we need time travel for.

50kb * 3 * 1000 = 150mb

Now let's think about a full screen canvas, just one, but it's taking about 500kb for that screenshot:

500kb * 1000 = 500mb

But that 1000 steps in a suite is actually quite conservative. This number includes every step to the test, and will very quickly exceed 1000 steps.

In addition to this, taking screenshots takes time and would noticeably slow the cypress runner down A LOT. It would also need to track which elements in the DOM these screenshots are in reference to, along with their dimensions. A canvas screenshot would commonly be much larger than the size of the DOM Canvas element due to common approaches to HDPI devices. This is a lot to track, and not something the DOM recorder is prepared for.

There is no logical end to any of these example numbers, and we're already under a tight memory constraint. Cypress is not a 'lightweight' solution, nor should it be.

IMHO this request would make Cypress worse than not including it at all. and I am a WebGL Canvas developer! I use cypress for canvas, not for DOM, and I wouldn't even want this feature. ( unless it was proven to be very robust and low impact, which I doubt is possible )

It is much better to be in control of when a screenshot is captured.

Also it's worth noting that Cypress' real power is running in headless mode for CI jobs, meaning that there is no time travel available anyhow. This feature would only be applicable to the visual runner itself.

@callumacrae
Copy link

callumacrae commented Jan 10, 2020

If you have a full screen canvas and cypress doesn't support taking snapshots of canvas, why would your test have 1000 steps?

Time travel isn't available on CI, but time travel isn't the only feature that uses snapshots - I use percy for visual regression tests which uses the snapshots from cypress and runs on CI. It would be super useful for the canvas elements to be compared in percy as well.

@stevenalanstark
Copy link

stevenalanstark commented Jan 10, 2020

@callumacrae - I don't quite understand your question, but what I think you're confused about is the arbitrary use of 1000 steps. My point is that the time travel steps quickly grow to very large numbers because every command is a new step. A test could conservatively have 1000+.

You have screenshot available to you, you just need to trigger them in your tests.

Actually, if you want to prove to yourself why this wouldn't be realistic as a core feature of Cypress time travel, just do this simple test:

  1. Load up the cypress kitchen sink example suite
  2. After each and every step in the test, manually take a fullscreen screenshot
  3. run the test.
  4. go make a sandwich, it's going to take much longer. ;)

I hope you can see my point here;

  1. it's better for you to be in control of when to take screenshot of canvas elements so that the test will remain fast, as well as having the screenshots in CI
  2. there is no way to add a feature like this into the dom recorder and keep cypress efficient because there is no reasonable limit to the size and number of screenshots that would need to be captured throughout the test.

@elhigu
Copy link

elhigu commented Jan 10, 2020

@stevenalanstark That comparison of speed is a bit unfair. Taking screenshot with cypress is slow, but just taking screenshot from canvas with toDataURL() is not so slow that it would affect much test run stepping speed (I had setup my cypress testsuite to store screenshots of my cavas with .toDataURL when I had problems with video recording feature).

@stevenalanstark
Copy link

stevenalanstark commented Jan 11, 2020

@elhigu yes . but then again I could argue that it's too lenient. many canvas applications are 2x ( or even 3x ) the size of the viewport for high pixel density devices. The point is that assuming the best case scenario will not be a viable approach here.

you do have a point that it is technically possible, and I could see this being a solo project as a NPM plugin. I'm just arguing against it being a core feature.

@elhigu
Copy link

elhigu commented Jan 11, 2020

@stevenalanstark downscaling to some maximum value is also easy and fast to do with secondary off screen canvas which is then used to take the screenshot (I do that for generating some thumbnails from webgl editor views). So it really doesn't matter that pixel density is high. After scaling the operation is still fast and amount of needed memory is limited.

The point is that assuming the best case scenario will not be a viable approach here.

Of course it is not. But in this case I don't see why there couldn't be a viable solution for restoring canvas images in time travel since I have already done similar solution manually on top of cypress (only missing thing was the integration to the timeline view so that those stored screenshots would have been visible).

Screenshots does not have to be full size ones and they doesn't have to be all kept in memory at the same time.

Actually, if you want to prove to yourself why this wouldn't be realistic as a core feature of Cypress time travel, just do this simple test:

My point was that in your earlier comment you were trying to push impossibility of implementing this feature with irrelevant test setup.

Still I'm not saying that general implementation for this would be easy, but still it would be viable.

At least some solution which would work for 95% of the use cases should be possible to implement.

I hope you can see my point here;

  1. it's better for you to be in control of when to take screenshot of canvas elements so that the test will remain fast, as well as having the screenshots in CI

  2. there is no way to add a feature like this into the dom recorder and keep cypress efficient because there is no reasonable limit to the size and number of screenshots that would need to be captured throughout the test.

  1. It would be better to have canvas image there without any special and slow cypress screenshot taking so that people wouldn't have to do it manually (as was reported originally in this issue)

  2. I don't see any reason why there would not be any way to do it. There can be some configurable limits for resolution and amount of images which can be selected by cypress user.

I don't understand why are you so sure and why are you trying so hard to prove that this would be impossible to implement.

@stevenalanstark
Copy link

stevenalanstark commented Jan 11, 2020

@elhigu - Sorry if I wasn't clear, in no way am I saying it's impossible. What I am saying is it is impractical. The only part that I said is impossible is the part about having time traveled canvas' be interactive, as in restoring their JS process to the exact state it was in while rendering the canvas. That's not possible here AFAIK.

All I'm showing is that if this were to be a core feature of Cypress, it would fall apart in a lot of common scenarios and that would harm Cypress as a product overall.

I fully support this sort of idea being pursued as a plugin to be added on a per - project basis.

@callumacrae
Copy link

@stevenalanstark you could opt out of this functionality simply by not taking snapshots with canvases in. Of course if you take a screenshot after every step in the cypress kitchen sink example it would be slow—but there aren't any canvases in the cypress kitchen sink example, which is sort of the point!

If someone is running cypress in a page with canvas on, they're probably doing that because they want to test the canvas element. Why else would they have tests on that page? At the moment that canvas is untested, and adding a test would slow down your tests and increase the memory usage. So does running tests against real APIs or external resources, and there's still a use case for that, it's just a compromise and up to the person writing the tests to make that decision.

I agree that it would increase the resource usage of the tests but disagree that the functionality shouldn't be added because of it. As long as the compromise is documented or behind an option I really don't see the harm in it. It wouldn't affect existing tests because you currently can't test canvas elements, so it's highly unlikely anybody has written tests for them.

@jennifer-shehane
Copy link
Member

Just wanted to note that we have a form of capturing canvas and replaying it via Test Replay if you are recording to the Cloud. Description here: #28289

I know this doesn't address this original issue. Capturing and replaying canvas is actually incredibly complicated and likely not possible to capture and redraw the state performantly, so if we did do this in the future, it would likely be generating a screenshot in its place like we did with Test Replay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: snapshot type: feature New feature that does not currently exist
Projects
None yet
Development

No branches or pull requests

8 participants