Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
@dubzzz is attempting to deploy a commit to the serhalp's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR enhances test coverage for the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/shared/utils/parse-basic-frontmatter.spec.ts (2)
152-186: Extract the PBT builders to keep the test callback under ~50 lines.This
itblock is well over the guideline size; moving the arbitraries into helpers or module-scope constants would keep the test focused and easier to scan.As per coding guidelines "Keep functions focused and manageable (generally under 50 lines)".
187-212: ConfigurenumRunsand size limits to keep CI time predictable.10,000 runs with unconstrained dictionary/array sizes can be expensive. Fast-check's recommended approach is to set a global time limit (
interruptAfterTimeLimit) in your test setup and makenumRunsconfigurable via environment variable for separate PR and nightly jobs. Additionally, thefc.dictionaryandfc.arraycalls here lack explicit size bounds—consider addingmaxLengthconstraints:Suggested changes
Set up global timeout in your test configuration (e.g.
vitest.config.ts):const fc = require('fast-check'); fc.configureGlobal({ numRuns: Number(process.env.FC_NUM_RUNS ?? 100), interruptAfterTimeLimit: process.env.CI ? 5_000 : undefined, // 5s in CI });Then update the arbitraries to cap sizes:
- const valueArb = fc.oneof( + const valueArb = fc.oneof( ...singleValueArbs(false), fc - .array(fc.oneof(...singleValueArbs(true)), { minLength: 1 }) + .array(fc.oneof(...singleValueArbs(true)), { minLength: 1, maxLength: 5 }) .map(arr => ({ raw: `[${arr.map(v => v.raw).join(', ')}]`, expected: arr.map(v => v.expected), })), ) - const frontmatterContentArb = fc.dictionary(keyArb, valueArb).map(dict => { + const frontmatterContentArb = fc.dictionary(keyArb, valueArb, { maxKeys: 10 }).map(dict => {
| @@ -1,4 +1,5 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
| import fc from 'fast-check' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check TS module interop flags
find . -name 'tsconfig*.json' -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Repository: npmx-dev/npmx.dev
Length of output: 769
🏁 Script executed:
# Check existing fast-check imports
rg "from ['\"]fast-check['\"]" --type=ts --type=tsx -nRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Search for existing fast-check imports without file type restriction
rg "from ['\"]fast-check['\"]" -nRepository: npmx-dev/npmx.dev
Length of output: 293
🏁 Script executed:
# Check if there's a test-specific tsconfig
find . -path '*/test/**' -name 'tsconfig*.json' -o -path '*/tests/**' -name 'tsconfig*.json'Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Look for vitest config and test setup
find . -name 'vitest.config.*' -o -name 'vite.config.*' | head -5Repository: npmx-dev/npmx.dev
Length of output: 79
Use namespace import to match existing test suite convention.
The codebase consistently uses import * as fc from 'fast-check' in other test files. Align with this established pattern.
Import update
-import fc from 'fast-check'
+import * as fc from 'fast-check'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import fc from 'fast-check' | |
| import * as fc from 'fast-check' |
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
we only need this to handle frontmatter we ourselves create
I would likely migrate us to use gray-matter in future, so it's probably not worth investing too much time fixing any issues in the function (which this almost certainly did find!)
|
Thanks @danielroe, I'll probably close this one then. No need to try to find bugs in it for now |
2 participants
I'm continuing my work on extending the usage of property based testing via fast-check to other parts of the codebase. Lately I wanted to play with
parseBasicFrontmatter. Given it was covered by classical tests I believed it was something useful to cover and make fully reliable.To date, I started with a very first version of a property based test. But it found some behaviors I was not expecting to encountered so I filtered some. Before continuing and cleaning up this PBT test that is now more and more complex to handle some edge-cases that might be bug I want to know if these edge-cases are expected or not.
In this PR, all the tests added without PBT are responsible to capture on of the edge-cases. The tests using
itare green, the ones withit.failsare red if we drop the.fails. Are these results expected?