Skip to content

Use vitest#expect from the local context#12347

Merged
vicb merged 5 commits intomainfrom
vicb/expect-miniflare
Feb 2, 2026
Merged

Use vitest#expect from the local context#12347
vicb merged 5 commits intomainfrom
vicb/expect-miniflare

Conversation

@vicb
Copy link
Copy Markdown
Contributor

@vicb vicb commented Feb 1, 2026

Part of #12346 - focus is on the miniflare package here

The code of this PR was authored by OpenCode/Opus


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: refactoring

A picture of a cute animal (not mandatory, but encouraged)


Open with Devin

@vicb vicb requested review from a team as code owners February 1, 2026 08:30
@vicb vicb added the miniflare Relating to Miniflare label Feb 1, 2026
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Feb 1, 2026

🦋 Changeset detected

Latest commit: c695c51

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Feb 1, 2026

Claude finished @vicb's task —— View job


Changeset Review

Tasks

  • Read changeset guidelines
  • Review changeset file
  • Check for cute animal images
  • Provide feedback

All changesets look good

The changeset for @cloudflare/eslint-config-shared (patch) is well-formatted and appropriate:

  • Version type: Patch is correct for adding a new ESLint rule
  • Description: Clear and explains both what the change does and why
  • Formatting: No problematic headers
  • Content: Appropriately concise and informative

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

View issues and 5 additional flags in Devin Review.

Open in Devin Review

Comment thread packages/miniflare/test/plugins/core/inspector-proxy/index.spec.ts
Comment thread packages/eslint-config-shared/rules/__tests__/no-vitest-import-expect.test.mjs Outdated
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Feb 1, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@12347

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@12347

miniflare

npm i https://pkg.pr.new/miniflare@12347

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@12347

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@12347

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@12347

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@12347

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@12347

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@12347

wrangler

npm i https://pkg.pr.new/wrangler@12347

commit: c695c51

@vicb vicb force-pushed the vicb/expect-miniflare branch from 05de6f8 to 11419d3 Compare February 1, 2026 08:47
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View issue and 5 additional flags in Devin Review.

Open in Devin Review

Comment thread packages/miniflare/test/plugins/core/inspector-proxy/index.spec.ts
Copy link
Copy Markdown
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread packages/pages-shared/eslint.config.mjs
@@ -1,3 +1,4 @@
// eslint-disable-next-line workers-sdk/no-vitest-import-expect
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why this exception? can you add a comment explaining it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is explained in more details in the linked #12346

The idea is to submit multiple larger but PRs convetring the simple/mechanical changes and keep the few changes that will need a more carful review for later - for now ignoring the rule for them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks 🙂, I'm happy with multiple PRs, but again potentially I'd add a TODO code comment linking to #12346 so that we don't forget to enable the rule elsewhere, but I'll leave this up to you 👍

Comment thread packages/miniflare/test/test-shared/worker-test.ts Outdated
R2_PLUGIN_NAME,
ReplaceWorkersTypes,
} from "miniflare";
// eslint-disable-next-line workers-sdk/no-vitest-import-expect
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why this exception? can you add a comment explaining it?

Comment thread packages/miniflare/test/plugins/local-explorer/helpers.ts Outdated
Comment thread packages/miniflare/test/plugins/kv/sites.spec.ts Outdated
Comment thread packages/miniflare/test/plugins/kv/index.spec.ts Outdated
Comment thread packages/miniflare/test/plugins/d1/suite.ts Outdated
Comment thread packages/miniflare/test/plugins/cache/index.spec.ts Outdated
@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Feb 1, 2026
@vicb
Copy link
Copy Markdown
Contributor Author

vicb commented Feb 1, 2026

lgtm

Hey Dario,

I didn't comment on exceptions because they are temp and will be removed soon (~days) as described in the linked issue #12346.

This is only a way to keep reviews reasonably sized and not too time consuming.

Does that sound good?

@dario-piotrowicz
Copy link
Copy Markdown
Member

Hey Dario,

I didn't comment on exceptions because they are temp and will be removed soon (~days) as described in the linked issue #12346.

This is only a way to keep reviews reasonably sized and not too time consuming.

Does that sound good?

Sounds good 🙂 (and you know that I really appreciate smaller / reviewable PRs 🙏 )

From my point of view a string replacement from:

// eslint-disable-next-line workers-sdk/no-vitest-import-expect

to

// eslint-disable-next-line workers-sdk/no-vitest-import-expect -- to be handled as part of https://github.com/cloudflare/workers-sdk/issues/12346

seems helpful and it doesn't seem very time consuming, but if you feel that it might be, or if you really find it very unnecessary/problematic, no problem, I'm ok with the code as is 🙂

(PS: I personally feel that todos are valuable even for very temporary changes, since you never know, something unexpected can come up and you might not be able to update the code as quickly as intended, in that case TODOs would still be there as a helpful reminder)

@vicb vicb merged commit 1a1f9e4 into main Feb 2, 2026
39 of 40 checks passed
@vicb vicb deleted the vicb/expect-miniflare branch February 2, 2026 07:09
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

miniflare Relating to Miniflare

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants