-
Notifications
You must be signed in to change notification settings - Fork 7
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
Next page button component tests - Clean branch #414
Next page button component tests - Clean branch #414
Conversation
(cherry picked from commit 443f742)
Sorry, don't have the time. Asking @rmanaem to take over! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good! One thing that @surchs wanted to know and I'd like to mention is why did you use the store getters and mutations instead of mocking them in the test?
Thanks @rmanaem for taking over. Like I said in the call, my question is whether we need to mock the getters by copying the actual logic into the test. I think one alternative is to just write the output that should get fed to the component as a literal, making the test clearer to understand. The other would be to reuse the getters in the store and then do more of an integration test. I prefer the former, the latter also works. But the problem I see with copying the whole logic here is that from reading the test I don't get a clear idea of what data goes into the component (missing out on clarity), and I also don't know if I'm in sync with the store (because the copy might have fallen out of sync). |
Understood @surchs and @rmanaem. The confusion here is that the next page component is a little bit different in that testing it could act like an integration test since its actual functionality is linked to the app state. However, this should be tested in our 'e2e' page tests and full e2e test. I'm revising the setup and tests now to be a proper component test for the |
Given the above comments from @surchs and @rmanaem, I opted to convert the tests away from integration-style and more to be component specific. This PR now includes tests for the *Does instruction text appear above the next page button when the button is disabled In addition, a small bug was noticed and this has been fixed in a separate commit. Instructions text that was to appear above the |
Just to note that this is currently failing the last component test on github only and not Cypress locally.
Currently investigating the cause. |
@jarmoza The last test Is breaking locally on my end too. |
@rmanaem Here's a mystery. |
@rmanaem What version of Cypress are you running locally? |
@jarmoza v12.9.0 |
With the update to Cypress 12, new cross-origin security measures prevent full component testing of navigation to a new page, particularly they block the click event from being tested. In this case, the click event for |
This seems to be quite interesting @jarmoza. Could you explain briefly what you have found or how Cypress is now preventing you from testing the button press when it passed earlier? I am assuming you are referring to the new addition of cross-origin testing: https://docs.cypress.io/guides/guides/cross-origin-testing ? edit: the reason I'm asking is that the cypress error message refers to a getter being written to, so it would be important to know that this is in fact somehow linked to cross-origin requests. |
Okay @rmanaem. Requested changes + some more cleanup committed. |
@surchs Yes, and this is a good reminder to write down/screen capture key error messages as we come across them. Thankfully we have one above. I'll try to summarize what Arman and I were seeing when attempting to test the request url and click event. The general consensus we reached was that once a click on the button was made the request to change url could be intercepted, but the click event itself was never fired. Depending on the order of the calls of intercept and spying on the store
The reason above why I was not initially seeing these errors was because I was using Cypress 10.3 locally. Once upgraded to 12.9, these errors above came up. You would think that moving from 'localhost:8080/' to 'localhost:8080/mynextpage' would have nothing to do with cross-origin handling since the origin should remain 'localhost', but as per error 3 and some stackoverflow reading about an error similar to 1 resolved by disabling a Cypress Chrome web security config field, it does seem to have something to do with a Cypress change between 10.3 and 12.9. As an addendum: The initial error on 10.3 I was experiencing was when the click tests were separated across two individual tests. Once a 'navigation' had been triggered via click, the Cypress testing environment would automatically fail the next test with an error reading something like |
(cherry picked from commit 443f742) Remaking this PR off a clean branch from dev.
This PR closes #412.
As mentioned in the issue, the following tests have now been implemented for the next-page component: