-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Replace argos-cli with @argos-ci/cli #34178
Conversation
372d472
to
947a44c
Compare
@michaldudak the Argos installation has been removed isn't it? |
@gregberge what do you mean by Argos installation? On a separate note - is the new SDK additionally compressing the images? Looking at this build (https://app.argos-ci.com/mui/material-ui/builds/4855), it seems that even though our screenshots are png, they have JPG artifacts introduced. This causes the CI to see changes from the previous (uncompressed, or compressed differently) version, even if the original screenshot is the same. |
@michaldudak please merge this |
Yeah the new SDK compresses it, it is normal. |
@michaldudak @oliviertassinari I have just added the support of Node.js v14 to |
.circleci/config.yml
Outdated
@@ -44,7 +44,7 @@ defaults: &defaults | |||
AWS_REGION_ARTIFACTS: eu-central-1 | |||
working_directory: /tmp/material-ui | |||
docker: | |||
- image: cimg/node:14.20 | |||
- image: cimg/node:16.17 |
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.
- image: cimg/node:16.17 | |
- image: cimg/node:14.20 |
I rather not change this because of #34178 (comment)
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.
This was done only to test if the new Argos SDK works. I don't intend to update Node in this PR.
We have made Argos CI as not required for the CI so we could continue to merge PRs.
@gregberge Is there a way we could disable it or have a much lower compression rate? With the artifacts of I would expect that changing the width of the Alert from 400px to 410px will change the artifacts all over the image, creating diffs where there is none. I also assume that this will make it possible to reduce the color difference threshold hence being able to detect more subtle real color changes (spot use of design tokens regressions). Sometimes, the compressed image is larger, e.g. I assume we would want to compress and pick the one that is smaller. The compression seems to make the upload time in the CI from ~7s to ~25s. This seems still OK. |
@oliviertassinari you are right, I had to check if I remove completely the compression or if I reduce it. |
Also, as we use PNGs, it's kind of pointless to compress the same image using JPEG. Could you perhaps use compression only when the input is a JPG? |
Problem is by default every screenshots are PNG, but there is no real transparency, so it is not optimal. Storage is expensive, I have to find solutions :) |
69995a9
to
60c0912
Compare
60c0912
to
fcded89
Compare
package.json
Outdated
@@ -69,6 +69,7 @@ | |||
"validate-declarations": "babel-node --extensions \".ts\" scripts/validateTypescriptDeclarations.ts" | |||
}, | |||
"devDependencies": { | |||
"@argos-ci/core": "^0.3.0", |
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.
@michaldudak a new version 0.4.0 has been released with a better compression algorithm. Could you please upgrade?
Also we have a Discord, feel free to join! https://discord.gg/vMyXWVA8
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'll try it right away, 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.
The build still fails the same way. It doesn't seem to depend on the version of CLI/SDK used.
Merged without explicit approval as it was blocking other PRs. |
Replaced the unsupported argos-cli with the new @argos-ci/cli package, as per argos-ci/argos#483 (comment).