Skip to content

upgrade vitest in libs/backend#6345

Merged
adghayes merged 4 commits intomainfrom
drew/vitest-libs-1
Apr 28, 2025
Merged

upgrade vitest in libs/backend#6345
adghayes merged 4 commits intomainfrom
drew/vitest-libs-1

Conversation

@adghayes
Copy link
Copy Markdown
Collaborator

@adghayes adghayes commented Apr 25, 2025

Overview

I thought to do a little mindless code contribution but actually this turned out to be a headache. I started with libs/backend, but because all the backends import test harnesses from libs/backend, conflicting package versions led me to upgrade all the backends. But because most of the backends import test utilities from libs/image-utils, I had to upgrade those and then other backends that import them.

In upgraded libraries, I've set the negative coverage thresholds to their maximum. The exception is pollbook/backend, which appears to have variable code coverage on every run, which seems bad, but is outside the scope of this PR.

@adghayes adghayes force-pushed the drew/vitest-libs-1 branch from 66b27ab to db8ac8d Compare April 25, 2025 23:33
Comment on lines +38 to +40
// eslint-disable-next-line @typescript-eslint/no-unused-vars
getAuthStatus: fn((machineState: InsertedSmartCardAuthMachineState) =>
Promise.resolve(InsertedSmartCardAuthTypes.DEFAULT_AUTH_STATUS)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The upgrade created a very strange test failure. In https://github.com/votingworks/vxsuite/blob/main/apps/mark/backend/src/app.ui_strings.test.ts, the last test started failing because, after the upgrade, the nested mock resets in the UI strings test harnesses were resetting. There are some breaking changes around spies, but I don't think any of them correspond exactly to what I was seeing. The reset will reset mocks to their original interpretation, which can be passed as an argument to vi.fn, which is what I do here. But there may be a larger issue with nested test harnesses resetting mocks in their calling test suites.


vi.setConfig({
testTimeout: 30000,
testTimeout: 60_000,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Maybe there was a perf hit as part of the upgrade, because I had to bump these timeouts substantially to make these pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I remember something to this effect with the upgrade I did as well. I see there are others having issues as well, and one of the sources of the performance regression was fixed in vitest-dev/vitest#7291. However, that was fixed in v3.0.3. Whether there are other perf issues lurking, I don't know.

Comment on lines +10 to +11
lines: -200,
branches: -119,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The actual number of uncovered lines varies from run to run, and this is a number which has passed so far 🤷

@adghayes adghayes requested review from arsalansufi and eventualbuddha and removed request for arsalansufi April 25, 2025 23:39
@adghayes adghayes marked this pull request as ready for review April 25, 2025 23:40
@adghayes adghayes requested a review from arsalansufi as a code owner April 25, 2025 23:40

vi.setConfig({
testTimeout: 30000,
testTimeout: 60_000,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I remember something to this effect with the upgrade I did as well. I see there are others having issues as well, and one of the sources of the performance regression was fixed in vitest-dev/vitest#7291. However, that was fixed in v3.0.3. Whether there are other perf issues lurking, I don't know.

@adghayes adghayes merged commit 6f92340 into main Apr 28, 2025
65 checks passed
@adghayes adghayes deleted the drew/vitest-libs-1 branch April 28, 2025 17:51
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.

2 participants