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

#234 Upgrade Vitest libraries to latest version #235

Merged
merged 13 commits into from
Sep 5, 2024

Conversation

nevendyulgerov
Copy link
Contributor

@nevendyulgerov nevendyulgerov commented Aug 26, 2024

I upgraded Vitest related packages to their latest versions and applied a couple of fixes for issues I noticed after the upgrade.

@nevendyulgerov nevendyulgerov self-assigned this Aug 26, 2024
Copy link

vercel bot commented Aug 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollups-explorer-base-mainnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm
rollups-explorer-base-sepolia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm
rollups-explorer-mainnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm
rollups-explorer-optimism-mainnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm
rollups-explorer-optimism-sepolia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm
rollups-explorer-sepolia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm
rollups-explorer-workshop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 3:21pm

@@ -4,9 +4,9 @@ import { defineConfig, UserConfig } from "vitest/config";
export default defineConfig({
plugins: [react()],
test: {
exclude: ["e2e/**"],
exclude: ["e2e/**", "node_modules/**"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the vitest command runs tests in node_modules if any exist, that's why I'm excluding this folder here.

"@testing-library/react": "^16.0.0",
"@types/node": "^20",
"@types/ramda": "^0.30.1",
"@types/react": "^18.3.3",
"@types/react-dom": "^18.3.0",
"@vitejs/plugin-react": "^4.1.0",
"@vitest/coverage-v8": "0.34.2",
"@vitest/coverage-v8": "^2.0.5",
"@vitest/ui": "^2.0.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this package so that we can use the Vitest UI if we want with the yarn test:ui script.

@@ -58,7 +58,7 @@ describe("connectionForm", () => {
).toBeInTheDocument();

expect(screen.getByTestId("icon-test-inactive")).toBeInTheDocument();
expect(screen.getByText("Save")).toBeInTheDocument();
expect(screen.getByTestId("connection-save")).toBeInTheDocument();
Copy link
Contributor Author

@nevendyulgerov nevendyulgerov Sep 2, 2024

Choose a reason for hiding this comment

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

More tests started failing after the latest rebase. Probably something in @testing-library/* or @mantine/* changed in a patch version, but I haven't dug into this further.

@nevendyulgerov
Copy link
Contributor Author

nevendyulgerov commented Sep 4, 2024

Hi, @brunomenezes , I checked the issue with the reduced test coverage.

From the looks of it, we are covering the same files with the inclusion of .ts files that contain only type definitions - interfaces, types, enums, etc. Example.

For those files the reporter is giving a 100% coverage though, so they are not bringing down the coverage.

Between this branch and the main branch I compared the same file (TransactionProgress):

Here's a screenshot:

Screenshot 2024-09-04 at 14 55 57

The only difference I noticed was that the individual imports are not marked with anymore. The remaining content of both files is identically covered. Also, both files have the same content. So it appears that the difference in the imports coverage is causing the percentage to go down. A confirmation for this is the fact that the number of uncovered lines in TransactionProgress (43) equals the number of lines for which there is no more a symbol:

Screenshot 2024-09-04 at 17 08 16

So, it appears that the reporter is giving different results for the same file contents.

I also tried a different reporter - @vitest/coverage-istanbul but with it, the coverage fell even more - https://coveralls.io/builds/69600513. Additionally, I checked vitest's repo for any related issues. I found this one and applied the suggested solution with ignoreEmptyLines but this didn't affect the coverage result.

So, I think we can either ignore the failed coveralls check for this PR and merge it like this with this new test coverage result or I can add more unit tests to prevent the decrease of test coverage.

@brunomenezes
Copy link
Collaborator

brunomenezes commented Sep 5, 2024

Hi, @brunomenezes , I checked the issue with the reduced test coverage.

@nevendyulgerov, that is alright. As we discussed, they changed a bit about how mapping works. That becomes the new threshold, and we will work from here.

@nevendyulgerov nevendyulgerov merged commit ea1a4c0 into main Sep 5, 2024
18 of 20 checks passed
@nevendyulgerov nevendyulgerov deleted the chore/234-upgrade-vitest-libraries branch September 5, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update vitest libraries
2 participants