-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Vitest prop testing #3703
Vitest prop testing #3703
Conversation
🦋 Changeset detectedLatest commit: 8e2d386 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
why not the following? it.effect.prop("should detect the substring", { a: Schema.String, b: Schema.String, c: Schema.String }, ({ a, b, c }) =>
Effect.gen(function*() {
return (a + b + c).includes(b)
})
) |
Yeah, I actually like that more. Updated. |
@mikearnaldi Mind having another look when you get the chance? |
Looks good to me! I'll let @tim-smart and @gcanti comment / review |
Whoops wrong PR. I'll review this one shortly. |
Hmm we can't depend on /schema as it isn't part of the core package at this stage. You can add it to a separate module however. |
Not even as a devDependency(for every other package besides /vitest)? |
It just can't be used in the /vitest/index.ts module, as /effect dogfoods it. So you can add it as /vitest/PropertyTesting, but will lose out on the chainable api. Or maybe it can extend the /index.ts apis in some way. |
72f7115
to
b8cca68
Compare
I moved it to /Schema and went with |
Then I would say let's hold off until we merge schema |
Hmm. Well this is awkward.. jaja. I've updated it to hopefully fix the issue. Still think we should wait? What's the timeline for merging schema? If it's w/ 4.0 maybe it's worth merging this version and then updating the api once schema is merged? |
Merging schema is not a breaking change for effect so most likely a matter or weeks |
Gotcha. Makes sense to revert to the original version and wait then. I'll ping this again when Schema gets merged. |
b8cca68
to
737109a
Compare
@tim-smart @mikearnaldi I rebased this against next-minor w/ schema merged into effect. Mind having another look? |
ae8526c
to
8410a20
Compare
packages/vitest/src/index.ts
Outdated
/** | ||
* @since 1.0.0 | ||
*/ | ||
export type SchemaObj<A, E, R> = Array<Schema.Schema<A, E, R>> | { [K in string]: Schema.Schema<A, E, R> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think you need any generics here. Also I don't think you can assign Schema.Never to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arbitrary doesn't support Schema.Never so I think not being able to assign Schema.Never to it is fine. But, yeah, this can be Schema.Any w/o the generics
effect/packages/schema/src/Arbitrary.ts
Lines 154 to 157 in b836fc4
case "NeverKeyword": | |
return () => { | |
throw new Error(errors_.getArbitraryUnsupportedErrorMessage(path, ast)) | |
} |
8410a20
to
9953830
Compare
Just need to rebase this again. |
Done :) |
Rebase, not merge, as there are a bunch of duplicate commits atm. |
5c98f44
to
8e2d386
Compare
There we go. Had to do some serious git magic to make that work jaja. |
Adds property testing.
https://www.npmjs.com/package/@fast-check/vitest