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

Upgrade react-icons to v5 #2117

Closed
yurishkuro opened this issue Jan 17, 2024 · 6 comments · Fixed by #2134
Closed

Upgrade react-icons to v5 #2117

yurishkuro opened this issue Jan 17, 2024 · 6 comments · Fixed by #2134

Comments

@yurishkuro
Copy link
Member

Dependabot creates a PR #2116 to upgrade react-icons to v5.

  1. This is a major release, we need help validating that all icons are still displayed sensibly
  2. The unit tests in the PR are breaking, probably due to some changes in the library that need to be investigated. It could be just out tests, or it could be that some APIs have changed.
@prajjwalyd
Copy link
Contributor

prajjwalyd commented Jan 24, 2024

Hello @yurishkuro,

After some investigation, I think I've identified the cause of the test failure...
It appears to be related to the rendering of SVG paths. The difference in the snapshot paths is notable:
for eg. in one case -
earlier, the path was d="M248 130a26 26 0 1026 26 26 26 0 00-26-26z", and now it is d="M248 130a26 26 0 1 0 26 26 26 26 0 0 0-26-26z" after updating the react-icons library.
The other cases are also similar and this discrepancy is specifically in how numbers and spaces are represented.

Upon reviewing the changelog of the react-icons library, I discovered an update in the svgo library in the version 5.0.0 that addresses these changes. Furthermore, the svgo library itself has undergone updates related to spaces and other improvements in version 3.0.3, which aligns with the version updated in the react-icons library.

I tried the yarn test-ci -- -u command to update the snapshots, and it resolved the error.

Please let me know if there are any additional considerations left to address in resolving this issue or if I might have missed anything in my analysis.


image

@yurishkuro
Copy link
Member Author

Can you spot-check those cases where snapshots change if there are any visual differences before and after?

@prajjwalyd
Copy link
Contributor

It appears that all of the failing cases are tied to a specific SVG path:
<path d="M248 130a26 26 0 1026 26 26 26 0 00-26-26z" key="3"/>

I visually compared the current path with the one generated after the react-icon library update by extracting the SVG into an HTML file.

BEFORE:
image

AFTER:
image


No observable differences were spotted. Therefore, I think updating the snapshots to reflect the new SVG path could be a valid solution.
If there are no objections, I can proceed with this course of action.
Additionally, we might consider exploring dedicated visual regression testing tools for these kind of issues.

For a closer look, I've prepared an HTML file for visual inspection. You can access it here.

@yurishkuro
Copy link
Member Author

  • where is this (i) icon used, in which pages/components?
  • looks good to proceed with a PR
  • visual regression testing sounds great, but I am not familiar with that area, how reliable they are, and what infra they require. An interesting area to investigate, for sure.

@prajjwalyd
Copy link
Contributor

  • Test failures are observed in the OperationTableDetails component, where the <IoInformationCircleOutline /> SVG icon from the react-icons library is utilized.
    While it seems that the library has made some changes to the SVG rendering in its recent version, a comparison between the old and new paths reveals no noticeable visual difference (as highlighted in my previous comment).
    Hence, I believe it's safe to proceed with updating the snapshots to accommodate the changes introduced by the updated react-icons library.
    Here is the preview of the icon (https://react-icons.github.io/react-icons/icons/io5/) -
    image

  • Should I include updates for both the snapshots and the react-icons library in my pull request?

  • I've been looking into visual regression testing tools like Jest-image-snapshot and Percy recently. I've learned that they are good at catching visual changes, but how well they work depends on factors like the quality of the baseline images, how consistent the test environment is, and the specifics of the application.
    I also have some background in end-to-end testing from a university course... So, if needed, I'm interested in taking on the task of integrating visual regression tools into our project in the future.

@yurishkuro
Copy link
Member Author

Should I include updates for both the snapshots and the react-icons library in my pull request?

yes, since otherwise the CI won't succeed.

yurishkuro pushed a commit that referenced this issue Jan 25, 2024
## Which problem is this PR solving?
Resolves #2117 

## Description of the changes
- I have upgraded the react-icons library to v5.0.1 which was earlier
attempted by Dependabot in PR #2116.
- A failing test occurred due to a snapshot mismatch after the upgrade.
I've documented my investigation in the comments of issue #2117. To
prevent any test failures, it is safe to update the existing snapshot.

## How was this change tested?
Using the `yarn test-ci` command in my local setup.

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have run lint and test steps successfully

---------

Signed-off-by: Prajjwal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants