[TS PBT] Execute Kotlin properties with fast-check - #375
Conversation
| const sourceRoot = sourceRoots[index]; | ||
| if (sourceRoot === undefined || !path.isAbsolute(sourceRoot)) { | ||
| throw protocolError( | ||
| 'source-root.invalid', |
There was a problem hiding this comment.
What are these strings? Do they occur randomly or the correponding matching exists somewhere? If it does, these lines should be places in some shared place with resources
There was a problem hiding this comment.
Fixed. The adapter-owned identifiers are centralized in diagnostics.ts. Each descriptor carries both its stable code and explicit category, so call sites contain no raw diagnostic codes and Kotlin does not infer categories from prefixes.
| } | ||
|
|
||
| export interface PropertyManifestWire { | ||
| schemaVersion: number; |
There was a problem hiding this comment.
We discussed that there is no need in schemaVersion and protocolVersion since this files won't be saved anywhere and the latest version will be always the one
There was a problem hiding this comment.
Removed schemaVersion and protocolVersion from the manifest and from both execution and projection request/response types. The private one-shot producer and consumer ship together, so there is no compatibility branch left.
| if (request.seed !== undefined && !isSignedInt(request.seed)) { | ||
| throw protocolError('protocol.seed.invalid', 'Seed must be a signed 32-bit integer', 'seed'); | ||
| } | ||
| if (request.replayPath !== undefined && |
There was a problem hiding this comment.
You must always extract such long and complicated conditions from the if condition
There was a problem hiding this comment.
Fixed. The compound check is now named invalidReplayPath before the if statement.
|
|
||
| function validateManifest(value: unknown): PropertyManifestWire { | ||
| const manifest = requireRecord(value, 'protocol.manifest.invalid', 'Manifest must be an object', 'manifest'); | ||
| const valid = manifest.schemaVersion === 1 |
There was a problem hiding this comment.
What is this? Why is it hardcoded with a particular version?
There was a problem hiding this comment.
Removed this schema-version check together with the schemaVersion field.
| return { name: input.name, domain: input.domain }; | ||
| }); | ||
| const validated: PropertyManifestWire = { | ||
| schemaVersion: 1, |
There was a problem hiding this comment.
Removed the hardcoded schemaVersion from the reconstructed manifest.
| } | ||
|
|
||
| private const val MAX_RUNS = 10_000 | ||
| private const val MAX_TIMEOUT_MILLIS = 86_400_000L |
There was a problem hiding this comment.
Why? What is this number?
There was a problem hiding this comment.
Removed the arbitrary one-day cap. The only upper bound now is Int.MAX_VALUE milliseconds, which matches the signed 32-bit delay supported by Node timers; that reason is documented in the configuration and design document.
|
|
||
| companion object { | ||
| private const val MAX_RUNS = 10_000 | ||
| private const val MAX_TIMEOUT_MILLIS = 86_400_000L |
There was a problem hiding this comment.
Why? And it is the same constant as in another file. Looks suspicious, are they related?
There was a problem hiding this comment.
Removed the duplicate backend constant. The CLI and backend now use PropertyRunConfiguration.MAX_TIMEOUT_MILLIS as the single Kotlin-side definition.
| } | ||
|
|
||
| private companion object { | ||
| const val PROCESS_IO_THREADS = 3 |
There was a problem hiding this comment.
Isn't it too little?
There was a problem hiding this comment.
Replaced the fixed three-thread pool with a cached executor. The client still submits exactly three independent tasks for stdin, stdout, and stderr, but scheduling no longer depends on a magic pool size.
| val value = validDoubleOrNull() ?: return false | ||
| val minimum = runCatching(domain.min::toDouble).getOrNull() ?: return false | ||
| val maximum = runCatching(domain.max::toDouble).getOrNull() ?: return false | ||
| return value in minimum..maximum |
There was a problem hiding this comment.
It is correct logic for numbers?
There was a problem hiding this comment.
Verified and covered with focused tests for range boundaries, positive and negative zero, fractions, NaN, infinities, and invalid encodings. Membership deliberately matches the values the corresponding fast-check arbitrary can generate: fc.integer excludes negative zero, while NumberDomain preserves binary64 special values according to its bounds and allowNaN.
|
|
||
| private fun JsConcreteValue.Number.validDoubleOrNull(): Double? = runCatching(::toDouble).getOrNull() | ||
|
|
||
| private const val NEGATIVE_ZERO_BITS = "8000000000000000" |
There was a problem hiding this comment.
Can we not hardcode it and use some functions to extract it?
There was a problem hiding this comment.
Fixed. Negative zero is now derived with Double.fromBits(Long.MIN_VALUE).toRawBits(), and values are checked through Double.toRawBits(); the hardcoded hexadecimal representation is gone.
Closes #348
Summary
Verification
npm testinusvm-ts-pbt/fast-check-adapter— 36 tests./gradlew :usvm-ts-pbt:check :usvm-ts-pbt:detekt :usvm-ts-pbt:installDist— 63 JVM tests, 0 detekt findings, distribution built