Visual-Regression-Tracker / agent-cypress

Visual Regression Tracker integration plugin
Apache License 2.0
74 stars 9 forks source link

"enableSoftAssert": true does not allow cy.vrtTrack() function to run in async and allow tests to run independently #170

Open alexniculae opened 2 years ago

alexniculae commented 2 years ago

Tested the below use case with the Cypress-Agent:

Is there any way to allow the tests to run async and independent of the engine of VRT, at least when the enableSoftAssert is set to true?

If we don't want to fail the tests because of the results of the visual test, then at least we could leverage the speed of the tests.

Let me know if this issue should actually be moved to the js-sdk - was not 100% sure, but that might be best 🙂

pashidlos commented 2 years ago

@alexniculae thanks for your report!

there is no restriction on async test execution using this option my bet that this is happening because of re-try mechanism that is currently does not respect enableSoftAssert option

could you try changing default retry number like here and check the timing

cy.vrtTrack("Whole page with additional options", {
  retryLimit: 0,
});
alexniculae commented 2 years ago

@pashidlos, thank you 🙂 the retries are already set to 0 in this case

what I see though is that with enableSoftAssert set to true or false the time it takes for the tests to run is the same

due to this matter, I assumed that with the option set to either true or false, the tests are waiting for the VRT response/result before being allowed to finish/proceed

i tried looking into the VRT code to see if there is anything that keeps the tests ‘blocked’ until VRT finishes processing, but am not perfectly sure if this logic could be coming from here: https://github.com/Visual-Regression-Tracker/agent-cypress/blob/c8f4775cef10d17bf7df5abdd3064f09fd7fe40d/lib/commands.ts#L51

so more rather then a bug, I’d see this ticket as a feature, to allow the (Cypress) tests to finish/proceed before VRT finishes its processing and replies with its results

so the VRT engine does its work in the background, independent from (in this case) Cypress, as the Cypress test is not dependent on the VRT result/response

looking forward for your thoughts

later edit: probably the Cypress agent should not wait for the VRT response though, only in the case where both of the following conditions are met:

enableSoftAssert: true && retryLimit: 0

pashidlos commented 2 years ago

@alexniculae you are right, the test is waiting the response from VRT in order to know the result not sure how this will work if image comparison result would be sent async so far I see one change for sure:

another thing is to make image comparison async - not sure if this is currently possible do you have any examples where it's done like this in other plugins or visual testing tool?

I would first try to review if it's possible to speed up response from VRT what do you think?

alexniculae commented 2 years ago

not sure how this will work if image comparison result would be sent async

@pashidlos, I see your point here; but in the following scenario:

enableSoftAssert: true && retryLimit: 0

there is no longer any reason to expect the answer from the image comparison; no matter what the result of the visual test would be, it would be irrelevant for the e2e test; or is there something that i didn’t see? 🙂

for the rest of the configuration options (e.g. enableSoftAssert: false OR retryLimit > 0), this would no longer be a valid option though, as (in this case) Cypress needs to know the result in order to handle the behavior.

possibly this is a simplistic approach, but a ternary to avoid waiting for the visual test result in the first configuration example above might save a few ms per print-screen

what do you think?

so far I see one change for sure:

  • ignore retry count if enableSoftAssert is true

I would avoid doing this, for the following scenario(s):

another thing is to make image comparison async - not sure if this is currently possible

from my point of view, this is only doable in the case where e2e tests don’t need to wait for the response from VRT (as per first point in the current comment)

I assume that the comparison engine only needs to receive the screenshot (+details/options) from the e2e test and then (if the e2e test does not need to wait for the visual test response) the engine does the comparison by itself and is in no way interdependent on the e2e test

so, again the (probably over-simplistic) solution of adding a decision of waiting or not for the response in some cases (enableSoftAssert: true && retryLimit: 0)

I would first try to review if it's possible to speed up response from VRT

that is also already great as well, but I see my proposal above as going great lengths to optimizing performance in some cases as well (such as the scenario of monitoring the visual tests independently of the e2e ones) 🙂

long message, but i wanted to share the details, as I am pretty sure we have different ways of using and leveraging the power of VRT, which is great, because good things come when mixing opinions and approaches 🙂

in case you’d like that we discuss this further, I’d even be glad to have a call on the topic

alexniculae commented 2 years ago

another way this could then be made easier for the users (but these are different issues that we need to consider and log in their own time) is that the visual tests would then no longer serve a purpose in blocking the e2e tests when running the image comparison, and could then run the comparison in async, and: