-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: support absolute path images when converting image to base64 #170
Conversation
@LironEr just bumping to see if you saw this so we can review together |
lib/enhanceReport.js
Outdated
@@ -147,8 +147,15 @@ function createVideoContext(video, baseFolder) { | |||
|
|||
function convertImageToBase64(screenshotsDir, imagePath) { | |||
const imgPath = path.join(screenshotsDir, imagePath); |
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.
it looks like imagePath
in your case is an absolute path, then it adds another absolute path, so you can add a check and if the variable is already absolute, use it as it is.
what do you think? it might solve the issue
If you add this part, I think you can revert the other parts, because imagePath
won't need to be inside screenshotsDir
, and the current NodeJS exception is pretty good.
const imgPath = path.join(screenshotsDir, imagePath); | |
const imgPath = path.isAbsolute(imagePath) ? imagePath : path.join(screenshotsDir, imagePath); |
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.
awesome! yeah makes sense
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 for the code review @LironEr i pushed your comment ! 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.
@LironEr i notice now my latest commit i removed the main part of the function convertImageToBase64() (whoops) i am adding it back but im testing it now and it is still failing for me with
Error: ENOENT: no such file or directory, open '\screenshot-compare.e2e.cy.ts\screenshot-compare.e2e.cy.ts -- users page - assert img (failed).png'
at Object.openSync (node:fs:596:3)
at Object.readFileSync (node:fs:464:35)
at convertImageToBase64 (C:\dev\frontend-e2e-cypress\node_modules\cypress-mochawesome-reporter\lib\enhanceReport.js:164:52)
at C:\dev\frontend-e2e-cypress\node_modules\cypress-mochawesome-reporter\lib\enhanceReport.js:147:13
at Array.map (<anonymous>)
======imgPath C:\dev\frontend-e2e-cypress\cypress\snapshots\screenshot-compare\screenshot-compare.e2e.cy.ts\__diff_output__\user list.diff.png
=====path.isAbsolute(imagePath)true
======imgPath \screenshot-compare.e2e.cy.ts\screenshot-compare.e2e.cy.ts -- users page - assert img (failed).png
=====path.isAbsolute(imagePath)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.
it looks like context.value[]
has this
\screenshot-compare.e2e.cy.ts\\\\screenshot-compare.e2e.cy.ts -- users page - assert img (failed).png
which is a valid path.isAbsolute()
according to https://www.w3schools.com/nodejs/met_path_isabsolute.asp..... context seems to not pass in the cypress screenshots dir in the path. would the code below be a bad idea?
function convertImageToBase64(screenshotsDir, imagePath) {
if (fse.pathExistsSync(imagePath)) {
return convertImg(imagePath);
} else {
const imgPath = path.join(screenshotsDir, imagePath);
return convertImg(imgPath);
}
function convertImg(pathToFile)
{
return 'data:image/png;base64, ' + fse.readFileSync(pathToFile, { encoding: 'base64' });
}
}
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.
@LironEr bump baybay bump
Thanks @kdp88 |
@LironEr Thanks for the help dude! |
hey all,
so currently im running into an issue when running a screenshot compare tool https://github.com/simonsmith/cypress-image-snapshot
from debugging the error :
i was getting i came to this
my suites [ ] has a context like this (cleaned up a bit for readability)
the screenshot compare tool adds its own directory outside of screenshots that i dont see being configurable
so when i console out
convertImageToBase64()
i get
so my thought was to try to see if an image exists in
imgPath
then do the conversion or just catch the error make it readable serve the errorThis isnt really solving the issue but im not sure there needs to be a solution to screenshots being added outside the cypress config var
screenshotsFolder