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
feature(testcafe): viewer public drive #1507
Conversation
Visual Review - Please review screenshots, then restart build. |
Visual Review - Please review screenshots, then restart build. |
a32e0e6
to
c15c4c4
Compare
Visual Review - Please review screenshots, then restart build. |
Visual Review - Please review screenshots, then restart build. |
Visual Review - Please review screenshots, then restart build. |
Visual Review - Please review screenshots, then restart build. |
120a874
to
c724512
Compare
Visual Review - Please review screenshots, then restart build. |
c724512
to
86cbec3
Compare
Visual Review - Please review screenshots, then restart build. |
Visual Review - Please review screenshots, then restart build. |
8eaf3e3
to
2fc0cda
Compare
Visual Review - Please review screenshots, then restart build. |
2fc0cda
to
c4cdd56
Compare
|
||
await this.waitForLoading() | ||
console.log(`Navigation into ${folderName} OK!`) | ||
} |
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.
Je serais plus pour diviser la méthode côté page-drive en 2. Car en soit, le goToFolder n'a pas à vérifier le breadcrumb... Le découpage des responsabilités est important
(à faire dans une autre PR, mais je note ça là quand même)
new RegExp('([' + numOfFiles + '].*){2}'), | ||
'Numbers of files uploaded does not match' | ||
) | ||
} |
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.
Idem découpler dans la page drive le fait de prendre le screenshot et le expect sur les nombres de fichier.
Par contre, je comprends pas ici pourquoi on fait tout ça ? On a juste besoin d'uploader les fichiers et faire un screenshot à la fin ? Pourquoi vérifier le contenu des H4 des modales etc ?
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.
Ces fonctions (Vérifier les modales, etc...) etaient deja en place avant l'ajout de VR, et je ne les ai pas enlevé pour pouvoir prendre le screenshot au bon moment.
Notament quand j'upload 25 fichiers à la suite : remplacer ces checks par un wait me semble plus propice à des erreurs
Par contre, c'est vrai que le execpt sur le texte peut etre retiré!
async goToFolder(folderName) { | ||
await t | ||
.expect(this.folderOrFileName.withText(folderName).exists) | ||
.ok(`No folder named ${folderName}`) |
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.
On n'a pas besoin de ce expect, non ?
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.
Ca reste dans les bonnes pratiques de testcafe de verifier l'existance d'un composant avant de l'utiliser (ici pour cliquer dessus)
Visual Review - Please review screenshots, then restart build. |
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.
Ping moi quand t'es dispo, je pense qu'il y a beaucoup de taff à faire sur cette PR encore :/
c4cdd56
to
7138aa1
Compare
Visual Review - Please review screenshots, then restart build. |
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.
Thanks! I think overall the changes look good, but there are some small problems.
///there is no photos on page | ||
await page.initPhotoCountZero() | ||
await page.uploadPhotos([`${DATA_PATH}/${IMG0}`]) | ||
|
||
await t.fixtureCtx.vr.takeScreenshotAndUpload('Upload-1-pic.png') | ||
await t.fixtureCtx.vr.takeScreenshotAndUpload('UploadImage/Upload-1-pic.png') |
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.
How comes sometimes we do if (t.fixtureCtx.isVR)
before taking a screenshot, and sometimes not?
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.
if (t.fixtureCtx.isVR)
is use in methods that were written before using VisualReview, so we don't have to duplicate them (ie : UploadFiles
is used in several tests : some already using VisualReview, and older tests which don't have screenshot yet, so we need a condition to know when to take screenshots)
Here, in photos_start_upload_photos.js
we are in a test
using VisualReview (as we do await initVR(..)
in .before
so we don't need to check the condition
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.
Ok I see, temporary code then. Thanks for the explanation!
Just a side-question: why are the files in |
Testcafe vocabulary (but we can change the name if we prefer) |
998d6fd
to
5afa2c4
Compare
Thanks! I now realize I may have asked this before :'). It's really confusing to me but if it's a convention I'm happy to keep it. Just might ask the same question in 3 month 😁 |
5afa2c4
to
9e70fa8
Compare
Visual Review - Please review screenshots, then restart build. |
a8ae69c
to
52b9f9d
Compare
Visual Review - Please review screenshots, then restart build. |
Visual Review - Please review screenshots, then restart build. |
fb6418a
to
6112d78
Compare
Visual Review - Please review screenshots, then restart build. |
3951383
to
43a688a
Compare
43a688a
to
635a72b
Compare
Visual Review - Please review screenshots, then restart build. |
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.
Thanks, LGTM!
new folder
drive-vr
containing page model, to use with visual testing :Some test function were modified to fit with visual review, and I didn't want to check the old tests which don't use Visual Review. In the future(?) all page model should move to
drive-vr
orphotos-vr
when all tests use Visual Reviewalso all methods which take a screenshot are now named
method_vr()