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] Replace argos-cli with @argos-ci/cli #34178

Merged
merged 7 commits into from Sep 7, 2022

Conversation

michaldudak
Copy link
Member

Replaced the unsupported argos-cli with the new @argos-ci/cli package, as per argos-ci/argos#483 (comment).

@michaldudak michaldudak added test dependencies Update of dependencies labels Sep 2, 2022
@mui-bot
Copy link

mui-bot commented Sep 2, 2022

No bundle size changes

Generated by 🚫 dangerJS against daab048

@gregberge
Copy link
Contributor

@michaldudak the Argos installation has been removed isn't it?

@michaldudak
Copy link
Member Author

@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.

@gregberge
Copy link
Contributor

@michaldudak please merge this

@gregberge
Copy link
Contributor

gregberge commented Sep 6, 2022

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.

Yeah the new SDK compresses it, it is normal.

@gregberge
Copy link
Contributor

gregberge commented Sep 6, 2022

@michaldudak @oliviertassinari I have just added the support of Node.js v14 to @argos-ci/core and @argos-ci/cli.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- image: cimg/node:16.17
- image: cimg/node:14.20

I rather not change this because of #34178 (comment)

Copy link
Member Author

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.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 6, 2022

We have made Argos CI as not required for the CI so we could continue to merge PRs.

Screenshot 2022-09-06 at 17 20 54


Yeah the new SDK compresses it, it is normal.

@gregberge Is there a way we could disable it or have a much lower compression rate? With the artifacts of

50b3d96f685127cabcc6f6871444ba5b1d0306bbca615512215ed39a87e04496

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.

Screenshot 2022-09-06 at 17 23 35

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.

@gregberge
Copy link
Contributor

@oliviertassinari you are right, I had to check if I remove completely the compression or if I reduce it.

@michaldudak
Copy link
Member Author

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?

@gregberge
Copy link
Contributor

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 :)

package.json Outdated
@@ -69,6 +69,7 @@
"validate-declarations": "babel-node --extensions \".ts\" scripts/validateTypescriptDeclarations.ts"
},
"devDependencies": {
"@argos-ci/core": "^0.3.0",
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member Author

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.

@michaldudak michaldudak merged commit 94ae268 into mui:master Sep 7, 2022
@michaldudak
Copy link
Member Author

Merged without explicit approval as it was blocking other PRs.

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants