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

test(DIST-136): Increase resilience for visual tests in firefox #2

Merged
merged 1 commit into from Apr 14, 2020

Conversation

antoniofull
Copy link
Contributor

@antoniofull antoniofull commented Apr 13, 2020

Increase resilience for visual tests

  • Refactor Embed Visual tests

  • Uses a non-animated form with an start button (the initial animation on the form causes the issue, since when the screenshot is taken the animation has started the elements are moving and so we have px changed each time)

  • Changes demo page title to be modified when the form is loaded (so we can test

  • Adds custom step that returns when the page was modified so we know the iframe is loaded

  • Disables tracking and animations on forms (although it does not seem to work)

  • Splits visual tests in mobile and desktop

  • Splits Travis job to run Firefox isolated in parallel

  • Makes all steps asynchronous

  • Removes Safari visual tests

  • Updates README

  • Explain the motivation for making this change.

  • Provide a test plan demonstrating that the code is solid.

  • Match the code formatting of the rest of the codebase.

  • Two other colleagues must approve this change.

  • QA have tested the solution provided.

  • Design and UX must validate this change.

@antoniofull antoniofull requested a review from a team April 13, 2020 13:18
Comment on lines +56 to +68
- yarn test:visual:mobile
- kill $EMBED_PID
- name: "Visual Tests Firefox (Internal)"
if: fork = false AND (branch = master OR branch = release)
script:
- yarn start --no-info & export EMBED_PID=$!
- yarn test:visual:install
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runs firefox visual tests in parallel on travis (speed up tests)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can run tests in parallel for each browser 👍 I suggest you run yarn test:visual:mobile separately as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good I wasn't sure if creating many jobs in parallel would cause issues, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think it would. Each job needs to clone the repo and install dependencies - if the task is short it usually should not have a separate job. If it takes longer than cloning and installing and has no dependencies it can be a separate job.

I think I would also split functional tests per browser to make it faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 give me 10 minutes and I'll push the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One build failed because both tests were running in parallel, I wonder if makes more sense to run one job with both tests in parallel (as @Gotusso set it up) or separated? I am not sure about it. Thoughts @mathio ? (right now I split them and removed the parallel flag)

Copy link
Collaborator

@mathio mathio Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try without the --parallel flag for cypress? But I would check with Franco first. So for now feel free to keep this as is.

Edit: I re-run the tests and they passed 🤷‍♂️ When run in separate Travis jobs it saves ~30 seconds of total run time.

README.md Show resolved Hide resolved
Comment on lines +4 to +7
// Changes the document title when the form is ready
if (event.data.type === 'form-ready') {
window.document.title = event.data.type
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the form is loaded, the page will receive a postMessage with data form-ready we change the title of the document to match the event, like so we have a better idea that the form is ready without having to dig in to the form itself.

@@ -27,7 +27,7 @@
width="100%"
height="100%"
frameborder="0"
src="https://admin.typeform.com/to/pDgR2Z"
src="https://admin.typeform.com/to/mzy0hT?disable-tracking=true"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use a static form without initial animation, since this seems to be the main issue

@cypress
Copy link

cypress bot commented Apr 13, 2020



Test summary

11 0 0 0


Run details

Project embed
Status Passed
Commit 6746522 ℹ️
Started Apr 14, 2020 2:10 PM
Ended Apr 14, 2020 2:11 PM
Duration 01:08 💡
OS Linux Ubuntu Linux - 14.04
Browser Firefox 67

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Comment on lines +29 to +38
},
async waitForIFrameMessage () {
this.executeAsyncScript(function (done) {
const interval = setInterval(function () {
if (window.document.title === 'form-ready') {
clearInterval(interval)
done()
}
}, 100)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use directly window object with codeceptjs and webdriver, so we use executeAsyncScript to run a function that checks at intervals when the title of the document is changed. see previous comments

@@ -33,7 +33,7 @@ export const setupIframeTesting = (iframe) => {
// We can only use it inside Chrome disabling web security --> see config
// Won't work in popups
if (Cypress.isBrowser('chrome')) {
getIframeBody(iframe).find('[data-qa="fixed-footer-progress"]').should('be.visible')
getIframeBody(iframe).find('[data-qa="start-button"]').should('be.visible')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Since we changed form, we still want to have some resilience to make sure the form is loaded internally, the new form has a start button so we check for it. This is for functional tests

Comment on lines +30 to +32
"test:visual:chrome": "yarn codeceptjs --config codecept-visual.conf.js run --steps --grep @desktop",
"test:visual:firefox": "yarn codeceptjs --config codecept-visual.conf.js run --steps --profile firefox --grep @desktop",
"test:visual:mobile": "yarn codeceptjs --config codecept-visual.conf.js run --steps --grep @mobile",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splits the visual tests to run separated for chrome, firefox and mobile

@antoniofull
Copy link
Contributor Author

Test summary

8 • 3 • 0 • 0

Run details

Project embed
Status Failed
Commit 5cd5a79 ℹ️
Started Apr 13, 2020 1:21 PM
Ended Apr 13, 2020 1:22 PM
Duration 00:53 💡
OS Linux Ubuntu Linux - 14.04
Browser Firefox 75
View run in Cypress Dashboard ➡️

Failures

right-drawer.spec.js Failed
1 Right Drawer Embed Widget > Closes Embed Right Drawer using Keyboard
drawer.spec.js Failed
1 Drawer Embed Widget > Closes Embed Drawer using Keyboard
popup.spec.js Failed
1 Popup Embed Widget > Closes Embed Popup using Keyboard
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@Typeform/distribution there is a known issue with Firefox75 looking in to it

@antoniofull antoniofull force-pushed the DIST-136-improve-firefox-visual-tests branch 2 times, most recently from 2689eb4 to 1ae8333 Compare April 13, 2020 13:52
@antoniofull
Copy link
Contributor Author

I have updated Travis to install Firefox v 67.0 which matches the one we use in Applitools

@antoniofull antoniofull force-pushed the DIST-136-improve-firefox-visual-tests branch 3 times, most recently from 2845cd0 to 0a23613 Compare April 13, 2020 15:00
Copy link
Contributor

@maxprilutskiy maxprilutskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, approved

.travis.yml Outdated Show resolved Hide resolved
Comment on lines +56 to +68
- yarn test:visual:mobile
- kill $EMBED_PID
- name: "Visual Tests Firefox (Internal)"
if: fork = false AND (branch = master OR branch = release)
script:
- yarn start --no-info & export EMBED_PID=$!
- yarn test:visual:install
Copy link
Collaborator

@mathio mathio Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try without the --parallel flag for cypress? But I would check with Franco first. So for now feel free to keep this as is.

Edit: I re-run the tests and they passed 🤷‍♂️ When run in separate Travis jobs it saves ~30 seconds of total run time.

@antoniofull
Copy link
Contributor Author

@mathio the tests pass, the question I think is more if we want to keep them in parallel (one travis job) or not ( separate travis jobs). While we might earn 30 seconds on the builds, keeping them parallels might be better solution for cypress logging/dashboard part.
I leave this pr open for now, let's check tomorrow with @Gotusso, is just a small detail I can change it quickly in both cases.

test(DIST-136): Fixes firefox75 bug

test(DIST-136): Fixes firefox version

test(DIST-136): Reverts functional tests

test(DIST-136): Split tests in separate jobs

test(DIST-136): Split tests in separate jobs

Update .travis.yml

Co-Authored-By: Matej Lednicky <matej.lednicky@typeform.com>

test(DIST-136): Run functional tests in parallel

Update .travis.yml

Co-Authored-By: Matej Lednicky <matej.lednicky@typeform.com>
@antoniofull antoniofull force-pushed the DIST-136-improve-firefox-visual-tests branch from ec99425 to c2c8e1c Compare April 14, 2020 14:06
@antoniofull antoniofull merged commit 0096d37 into master Apr 14, 2020
@antoniofull antoniofull deleted the DIST-136-improve-firefox-visual-tests branch April 14, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants