-
Notifications
You must be signed in to change notification settings - Fork 123
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
Conversation
- 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 |
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.
Runs firefox visual tests in parallel on travis (speed up tests)
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.
Yes, we can run tests in parallel for each browser 👍 I suggest you run yarn test:visual:mobile
separately as well.
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.
Sounds good I wasn't sure if creating many jobs in parallel would cause issues, thanks!
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.
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.
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.
👍 give me 10 minutes and I'll push the changes
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.
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)
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.
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.
// Changes the document title when the form is ready | ||
if (event.data.type === 'form-ready') { | ||
window.document.title = event.data.type | ||
} |
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.
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" |
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.
We use a static form without initial animation, since this seems to be the main issue
Test summaryRun details
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 |
}, | ||
async waitForIFrameMessage () { | ||
this.executeAsyncScript(function (done) { | ||
const interval = setInterval(function () { | ||
if (window.document.title === 'form-ready') { | ||
clearInterval(interval) | ||
done() | ||
} | ||
}, 100) | ||
}) |
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.
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') |
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.
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
"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", |
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.
Splits the visual tests to run separated for chrome, firefox and mobile
@Typeform/distribution there is a known issue with Firefox75 looking in to it |
2689eb4
to
1ae8333
Compare
I have updated Travis to install Firefox v 67.0 which matches the one we use in Applitools |
2845cd0
to
0a23613
Compare
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.
Looks good to me, approved
- 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 |
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.
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.
@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. |
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>
ec99425
to
c2c8e1c
Compare
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.