Conversation
- made `PropCheck` a `func`
- start tests for `set`s
add extra overloads upto five arbitraries reduce redundant code in `execProperty` thanks to Zerbina's suggestion
this doesn't seem like the right approach
this allows for replication
If it makes sense in the future it'll naturally pop out of a refactoring
still debating if numbering per section is senseible, or should each property have its own number for easy reference?
|
The output from the current tests looks as follows, each property has an |
some test checks were weaker than they could be, only checking if failure was detected even if failure was guaranteed. clean-up: - trailing white spaces - superfluous comments
|
There are places for improvement but I'm not sure exactly where to take it without more experience with the library. So with that said I think this is ready to be reviewed and merged. |
|
I'm heavily reworking the tests:
The above found some bugs, so I'll flush those out as well. |
| for (pos, kind) in nodes: | ||
| if kind in {skArray8, skArray16, skArray32}: | ||
| let lenBytes = | ||
| case kind | ||
| of skArray8: 1 | ||
| of skArray16: 2 | ||
| of skArray32: 4 | ||
| else: unreachable("Invalid kind: " & $kind) |
There was a problem hiding this comment.
| for (pos, kind) in nodes: | |
| if kind in {skArray8, skArray16, skArray32}: | |
| let lenBytes = | |
| case kind | |
| of skArray8: 1 | |
| of skArray16: 2 | |
| of skArray32: 4 | |
| else: unreachable("Invalid kind: " & $kind) | |
| for (pos, kind) in nodes: | |
| if kind == skArray8 or kind == skArray16 or kind == skArray32: | |
| let lenBytes = | |
| case kind | |
| of skArray8: 1 | |
| of skArray16: 2 | |
| of skArray32: 4 | |
| else: unreachable("Invalid kind: " & $kind) |
Update partially good news is that the error is actually intermitent, so some other memory corruption is happening... yay, I still don't know where though.
I'm currently hitting the unreachable unless I change the code to the above, and I must be blind because I don't see why that's happening.
The cgen seems fine:
cgen for or variant:
NU8* kind;
FR_.line = (NI64)898;
FR_.filename = "property_testing.nim";
kind = (&_107->Field1);
NIM_BOOL _108;
_108 = NIM_FALSE;
NIM_BOOL _109;
_109 = NIM_FALSE;
FR_.line = (NI64)899;
_109 = ((*kind) == (NU8)9);
NIM_BOOL _111;
_111 = !_109;
if (_111) {
_109 = ((*kind) == (NU8)10);
}
_108 = _109;
NIM_BOOL _113;
_113 = !_108;
if (_113) {
_108 = ((*kind) == (NU8)11);
}
if (_108) {cgen for in variant:
kind = (&_107->Field1);
NIM_BOOL _109;
FR_.line = (NI64)899;
_109 = !(((NU16)3584 & ((NU16)1 << ((NU16)(*kind)))) == (NU16)0);
if (_109) {
zerbina
left a comment
zerbina
left a comment
There was a problem hiding this comment.
I did a first review pass, focusing on the things that can cause behaviour that looks like memory corruption.
| let rMax = decodeUint64(buffer, pos, sBytes) | ||
| pos += sBytes | ||
| let rangeSize = if rMax > rMin: rMax - rMin else: 0'u64 | ||
| pos += bytesForRange(rangeSize) |
There was a problem hiding this comment.
| pos += bytesForRange(rangeSize) | |
| pos += bytesForRange(rMax - rMin) |
If rMax < rMin (happens for signed integer ranges crossing zero), pos wouldn't be advanced properly when bytesForRange(rMax - rMin) != 1.
| rMin = decodeUint64(buffer, pos + 2, sBytes) | ||
| rMax = decodeUint64(buffer, pos + 2 + sBytes, sBytes) | ||
| rangeSize = if rMax > rMin: rMax - rMin else: 0'u64 | ||
| vBytes = bytesForRange(rangeSize) |
There was a problem hiding this comment.
| vBytes = bytesForRange(rangeSize) | |
| vBytes = bytesForRange(rMax - rMin) |
Same as above.
There was a problem hiding this comment.
I think with the conversion I have in place this should no longer be an issue.
There was a problem hiding this comment.
It doesn't cause any issues anymore, yea, but the rMax > rMin check is still unnecessary.
| if kind in {skByte, sk2Bytes, sk4Bytes, sk8Bytes}: | ||
| let sBytes = getScalarBytes(kind) | ||
| if pos + 1 + sBytes <= buffer.len: | ||
| let |
There was a problem hiding this comment.
Since the scalar reduction is identical to the one used by range reduction (and also has the same issue with yielding some candidates twice), could you factor the code out into a template?
There was a problem hiding this comment.
I've pulled out the core of it into numberShrinker, I'll see if I can rework the code to make them more similar to pull a bit more in tomorrow. 🤞🏽
|
Note to self: drop bool strings, they're not implemented and would be annoying to deal with, and the savings are questionable |
- introduce `int64` `chooseRange` - create a conversion function from `uint64` to `int64` that preserves order - fix-up call sites and tests
focuses on `candidates` iterator, to avoid misreading the buffer
These should change from `doAssert` to `assert` Also, consider allowing catchable assertions for facilitating testing.
Currently buggy, it seems `collectNodes` isn't gathering all the nodes
|
|
||
| # MARK: Buffer Tree Tools | ||
|
|
||
| proc treeRepr*(buffer: seq[byte]): string = |
There was a problem hiding this comment.
Note to self: debug why this, and/or collectNodes, is not printing the tree properly in the failing test
There was a problem hiding this comment.
In case you didn't get to debug it yet, this is because the buffer doesn't start with an array or group node, meaning that collectNodes only yields the node itself (a range).
There was a problem hiding this comment.
I got that visualized, I'm just trying to sort out where it's happening now:
# what collect nodes sees
@[(0, skRange)]
# the buffer; note `5` corresponds to `skRange` and it's missing group
@[5, 2, 1, 0, 0, 0, 10, 0, 0, 0, 1, 6, 2, 9, 2, 5, 3, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 5, 2, 1, 0, 0, 0, 100, 0, 0, 0, 0, 6, 0, 9, 2, 5, 3, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 5, 2, 1, 0, 0, 0, 100, 0, 0, 0, 0, 6, 1, 5, 0, 0, 255, 65]
# what my repr ends up outputing, because of what collect sees
skRange (kind: sk4Bytes, min: 1, max: 10, val: 1)
There was a problem hiding this comment.
The structure comes to be because in genSeq, there's a chooseRange call (for computing the length to use) before beginArray, adding a superfluous node to the tree.
The best way I can think of to address this is to add two more fields to the skArrayX nodes, one for the minimum and one for the maximum length. This would also fix shrinking being able to produce candidates violating the minimum length specification.
There was a problem hiding this comment.
I've started reworking the API, I collapsed all arrays to one kind, skArray, and made the length an skRange. The beginArray API now takes a min and max and so the range should be created in the correct position, it always write the array kind first and it returns the chosen length. This has the side effect of making getting rid of a lot recording state checks being dropped from various callers.
Now I gotta debug various bits and bobs, such as minimum length violations and whatever bugs lie in my rework.
Might be better to keep the various byte width arrays but they're easy enough to introduce after, I believe. It would save me a range kind byte.
Since it creates an `skGroup` and that only takes a byte worth of elements this makes the API safer. suggested by @zerbina
also renamed them to `renumerateXtoY` as renumerate means to renumber/recount, instead of conversion which is more ambiguous.
- work even if the input expression (`x`) has side-effects - avoid conversions that would result in range check errors and potentially extra work Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
need to fix `collectNodes` still
|
I'm going to be pushing a lot of changes that are incremental as I rework the internals, so I've marked it as draft for now so as to not burn CI resources unnecessarily. |
This test is the next failure to fix: > candidates yields structurally valid byte sequences (Meta-test) It shows that int -> uint range encoding is busted see the `treeRepr` output (confirmed by mentally converting the byte buffer as well): > skRange (kind: sk8Bytes, min: 9223372036854774808, max: 9223372036854776808, offset: 1741, value: 9223372036854776549) code is still ugly: - need to create a single range/array unpack template - array shouldn't have an skrange following it, just the kind, min, max, and offset
the array shrinking is more correct wherein `rMax and `rMin` aren't as far off in the final `skipToRangeOffsetAndGetSizeAndMin` template.
2 participants
Summary
Introduces experimental property based testing support via the
property_testingmodule in the stdlib, available under C and JS backends.Details
Create an integrated shrinking based approach to property based testing. This is based off the work in Python Hypothesis test library by Dr David R. Maciver.
The key benefits of this approach are:
The library ships with a number of basic generators, for scalar primitives such as numerics, booleans, characters, and the like. As well as vector like types, such as arrays, sequences, sets, and so on. Also generators for Tuples and procedural values (must be closures). In addition to these types a variety of combinators,
map,filter, etc have been provided to allow composition of generators to create These provide the foundational primitives to compose arbitrarily complex generation of data types that a user might want.Notes for Reviewers