Add unit tests for libexpr by andir · Pull Request #5377 · NixOS/nix

andir · 2021-10-12T19:53:29Z

This is an initial step towards working on the libexpr code and trying
to optimize it. Having unit tests will be a great starting point to
write a few benchmarks that focus on various features of NIx. Those
might not have to be part of the code base but are likely very helpful
while developing.

thufschmitt

That’s great :)

(The tests here are probably way to simple to be really useful, but it’s great to have a foundation for writing more of these).

I think something must be done about the EvalState issue, but apart from that the comments are rather minor

thufschmitt · 2021-10-13T11:23:42Z

src/libexpr/tests/primops.cc

+                : store(openStore("dummy://"))
+                , state({"."}, store)


This will probably be problematic as EvalState is inherently stateful, so having several instances of it at the same time is very likely to break.
Maybe we should instead have only one global EvalState for the whole testsuite as long as reentrance isn’t guaranteed (which obviously would be detrimental for purity, but would prevent a whole class of false-positives)

Could you point out where it is stateful beyond its own lifetime? Happy to start reducing the statefulness of it as part of this work.

I've now used this extensively and with that one primOp fix applied I've not seen any issues. I used this work to fuzz the parser multiple thousand times per second within one process for a few days without issues. If there is still some global state (besides the GC) I can't find it....

src/libexpr/tests/primops.cc

thufschmitt · 2021-10-13T11:36:28Z

(Also: I think you accidentally committed the binary)

andir · 2021-10-13T13:06:47Z

(Also: I think you accidentally committed the binary)

Yeah. This was pretty much a WIP commit after my evening activities yesterday. Just wanted to put it out there in case I never get back to it and someone else wants to pick up on that. I've once before written these tests (and a dummy store implementation) but never submitted the PR... :/

I'll try to go through your review comments in the evening.

Meanwhile: The EvalState teardown seems to actually work. I've added a couple of tests (same content, just different names) to do a smoke test if things start failing after a "magic" number of tests. They didn't so that was good. In any case the tests using the primop_sub are failing if executed one after another. I haven't traced this further but somewhere in the execution the value of the internalType of one of the arguments is an invalid number. I've tried the same with GC_DONT_GC set to rule out the GC doing weird stuff. Surprisingly the internvalType value is always 527810692. I'll try running it through rr tonight to see where and why that is being set. It does smell a bit.

On the EvalState argument: I'd rather work towards a setup where we can have multiple of those instances. I would like this approach to be useful for things like fuzzing the Nix language/interpreter and requiring a common shared state or process restart isn't really going to cut it. Parsing will leak memory (which will be a bit of a downer for fuzzing etc..) but that is probably tolerable for now.

vobloeb · 2021-10-13T20:42:43Z

https://github.com/NixOS/nix/tree/master/tests/lang might be rewritten to C++ with gtest

andir · 2021-10-13T23:33:13Z

https://github.com/NixOS/nix/tree/master/tests/lang might be rewritten to C++ with gtest

I purposely want the simple expressions as well. They are fast and if they break something is fundamentally broken. Not sure if makes sense to move those tests over. Perhaps in the long run but that isn't my call.

vobloeb · 2021-10-13T23:39:29Z

https://github.com/NixOS/nix/tree/master/tests/lang might be rewritten to C++ with gtest

I purposely want the simple expressions as well. They are fast and if they break something is fundamentally broken. Not sure if makes sense to move those tests over. Perhaps in the long run but that isn't my call.

I meant that in that case we could see the test coverage of libexpr and what tests need be added

andir · 2021-11-06T18:46:12Z

Picking up my debugging quest I've found out that the primOps aren't always registered with the correct arity when more than one EvalState is being initialized:

[ RUN      ] TrivialExpression.true
Registering primOp __getFlake with arity 1
Registering primOp scopedImport with arity 2
Registering primOp import with arity 1
Registering primOp __typeOf with arity 1
Registering primOp isNull with arity 1
Registering primOp __isFunction with arity 1
Registering primOp __isInt with arity 1
Registering primOp __isFloat with arity 1
Registering primOp __isString with arity 1
Registering primOp __isBool with arity 1
Registering primOp __isPath with arity 1
Registering primOp __genericClosure with arity 1
Registering primOp abort with arity 1
Registering primOp throw with arity 1
Registering primOp __addErrorContext with arity 2
Registering primOp __ceil with arity 1
Registering primOp __floor with arity 1
Registering primOp __tryEval with arity 1
Registering primOp __getEnv with arity 1
Registering primOp __seq with arity 2
Registering primOp __deepSeq with arity 2
Registering primOp __trace with arity 2
Registering primOp derivationStrict with arity 1
Registering primOp placeholder with arity 1
Registering primOp __toPath with arity 1
Registering primOp __storePath with arity 1
Registering primOp __pathExists with arity 1
Registering primOp baseNameOf with arity 1
Registering primOp dirOf with arity 1
Registering primOp __readFile with arity 1
Registering primOp __findFile with arity 2
Registering primOp __hashFile with arity 2
Registering primOp __readDir with arity 1
Registering primOp __toXML with arity 1
Registering primOp __toJSON with arity 1
Registering primOp __fromJSON with arity 1
Registering primOp __toFile with arity 2
Registering primOp __filterSource with arity 2
Registering primOp __path with arity 1
Registering primOp __attrNames with arity 1
Registering primOp __attrValues with arity 1
Registering primOp __getAttr with arity 2
Registering primOp __unsafeGetAttrPos with arity 2
Registering primOp __hasAttr with arity 2
Registering primOp __isAttrs with arity 1
Registering primOp removeAttrs with arity 2
Registering primOp __listToAttrs with arity 1
Registering primOp __intersectAttrs with arity 2
Registering primOp __catAttrs with arity 2
Registering primOp __functionArgs with arity 1
Registering primOp __mapAttrs with arity 2
Registering primOp __isList with arity 1
Registering primOp __elemAt with arity 2
Registering primOp __head with arity 1
Registering primOp __tail with arity 1
Registering primOp map with arity 2
Registering primOp __filter with arity 2
Registering primOp __elem with arity 2
Registering primOp __concatLists with arity 1
Registering primOp __length with arity 1
Registering primOp __foldl' with arity 3
Registering primOp __any with arity 2
Registering primOp __all with arity 2
Registering primOp __genList with arity 2
Registering primOp __sort with arity 2
Registering primOp __partition with arity 2
Registering primOp __concatMap with arity 2
Registering primOp __add with arity 2
Registering primOp __sub with arity 2
Registering primOp __mul with arity 2
Registering primOp __div with arity 2
Registering primOp __bitAnd with arity 2
Registering primOp __bitOr with arity 2
Registering primOp __bitXor with arity 2
Registering primOp __lessThan with arity 2
Registering primOp toString with arity 1
Registering primOp __substring with arity 3
Registering primOp __stringLength with arity 1
Registering primOp __hashString with arity 2
Registering primOp __match with arity 2
Registering primOp __split with arity 2
Registering primOp __concatStringsSep with arity 2
Registering primOp __replaceStrings with arity 3
Registering primOp __parseDrvName with arity 1
Registering primOp __compareVersions with arity 2
Registering primOp __splitVersion with arity 1
Registering primOp __unsafeDiscardStringContext with arity 1
Registering primOp __hasContext with arity 1
Registering primOp __unsafeDiscardOutputDependency with arity 1
Registering primOp __getContext with arity 1
Registering primOp __appendContext with arity 2
Registering primOp fetchMercurial with arity 1
Registering primOp fetchTree with arity 1
Registering primOp __fetchurl with arity 1
Registering primOp fetchTarball with arity 1
Registering primOp fetchGit with arity 1
Registering primOp fromTOML with arity 1
[       OK ] TrivialExpression.true (12 ms)
[ RUN      ] TrivialExpression.false
Registering primOp __getFlake with arity 1
Registering primOp scopedImport with arity 2
Registering primOp import with arity 0
Registering primOp __typeOf with arity 0
Registering primOp isNull with arity 0
Registering primOp __isFunction with arity 0
Registering primOp __isInt with arity 0
Registering primOp __isFloat with arity 0
Registering primOp __isString with arity 0
Registering primOp __isBool with arity 0
Registering primOp __isPath with arity 0
Registering primOp __genericClosure with arity 1
Registering primOp abort with arity 0
Registering primOp throw with arity 0
Registering primOp __addErrorContext with arity 2
Registering primOp __ceil with arity 0
Registering primOp __floor with arity 0
Registering primOp __tryEval with arity 0
Registering primOp __getEnv with arity 0
Registering primOp __seq with arity 0
Registering primOp __deepSeq with arity 0
Registering primOp __trace with arity 0
Registering primOp derivationStrict with arity 1
Registering primOp placeholder with arity 0
Registering primOp __toPath with arity 0
Registering primOp __storePath with arity 0
Registering primOp __pathExists with arity 0
Registering primOp baseNameOf with arity 0
Registering primOp dirOf with arity 0
Registering primOp __readFile with arity 0
Registering primOp __findFile with arity 2
Registering primOp __hashFile with arity 0
Registering primOp __readDir with arity 0
Registering primOp __toXML with arity 0
Registering primOp __toJSON with arity 0
Registering primOp __fromJSON with arity 0
Registering primOp __toFile with arity 0
Registering primOp __filterSource with arity 0
Registering primOp __path with arity 0
Registering primOp __attrNames with arity 0
Registering primOp __attrValues with arity 0
Registering primOp __getAttr with arity 0
Registering primOp __unsafeGetAttrPos with arity 2
Registering primOp __hasAttr with arity 0
Registering primOp __isAttrs with arity 0
Registering primOp removeAttrs with arity 0
Registering primOp __listToAttrs with arity 0
Registering primOp __intersectAttrs with arity 0
Registering primOp __catAttrs with arity 0
Registering primOp __functionArgs with arity 0
Registering primOp __mapAttrs with arity 0
Registering primOp __isList with arity 0
Registering primOp __elemAt with arity 0
Registering primOp __head with arity 0
Registering primOp __tail with arity 0
Registering primOp map with arity 0
Registering primOp __filter with arity 0
Registering primOp __elem with arity 0
Registering primOp __concatLists with arity 0
Registering primOp __length with arity 0
Registering primOp __foldl' with arity 0
Registering primOp __any with arity 0
Registering primOp __all with arity 0
Registering primOp __genList with arity 0
Registering primOp __sort with arity 0
Registering primOp __partition with arity 0
Registering primOp __concatMap with arity 0
Registering primOp __add with arity 0
Registering primOp __sub with arity 0
Registering primOp __mul with arity 0
Registering primOp __div with arity 0
Registering primOp __bitAnd with arity 0
Registering primOp __bitOr with arity 0
Registering primOp __bitXor with arity 0
Registering primOp __lessThan with arity 0
Registering primOp toString with arity 0
Registering primOp __substring with arity 0
Registering primOp __stringLength with arity 0
Registering primOp __hashString with arity 0
Registering primOp __match with arity 0
Registering primOp __split with arity 0
Registering primOp __concatStringsSep with arity 0
Registering primOp __replaceStrings with arity 0
Registering primOp __parseDrvName with arity 0
Registering primOp __compareVersions with arity 0
Registering primOp __splitVersion with arity 0
Registering primOp __unsafeDiscardStringContext with arity 1
Registering primOp __hasContext with arity 1
Registering primOp __unsafeDiscardOutputDependency with arity 1
Registering primOp __getContext with arity 1
Registering primOp __appendContext with arity 2
Registering primOp fetchMercurial with arity 1
Registering primOp fetchTree with arity 1
Registering primOp __fetchurl with arity 0
Registering primOp fetchTarball with arity 0
Registering primOp fetchGit with arity 0
Registering primOp fromTOML with arity 1
[       OK ] TrivialExpression.false (1 ms)
[ RUN      ] TrivialExpression.null
Registering primOp __getFlake with arity 1
Registering primOp scopedImport with arity 2
Registering primOp import with arity 0
Registering primOp __typeOf with arity 0
Registering primOp isNull with arity 0
Registering primOp __isFunction with arity 0
Registering primOp __isInt with arity 0
Registering primOp __isFloat with arity 0
Registering primOp __isString with arity 0
Registering primOp __isBool with arity 0
Registering primOp __isPath with arity 0
Registering primOp __genericClosure with arity 1
Registering primOp abort with arity 0
Registering primOp throw with arity 0
Registering primOp __addErrorContext with arity 2
Registering primOp __ceil with arity 0
Registering primOp __floor with arity 0
Registering primOp __tryEval with arity 0
Registering primOp __getEnv with arity 0
Registering primOp __seq with arity 0
Registering primOp __deepSeq with arity 0
Registering primOp __trace with arity 0
Registering primOp derivationStrict with arity 1
Registering primOp placeholder with arity 0
Registering primOp __toPath with arity 0
Registering primOp __storePath with arity 0
Registering primOp __pathExists with arity 0
Registering primOp baseNameOf with arity 0
Registering primOp dirOf with arity 0
Registering primOp __readFile with arity 0
Registering primOp __findFile with arity 2
Registering primOp __hashFile with arity 0
Registering primOp __readDir with arity 0
Registering primOp __toXML with arity 0
Registering primOp __toJSON with arity 0
Registering primOp __fromJSON with arity 0
Registering primOp __toFile with arity 0
Registering primOp __filterSource with arity 0
Registering primOp __path with arity 0
Registering primOp __attrNames with arity 0
Registering primOp __attrValues with arity 0
Registering primOp __getAttr with arity 0
Registering primOp __unsafeGetAttrPos with arity 2
Registering primOp __hasAttr with arity 0
Registering primOp __isAttrs with arity 0
Registering primOp removeAttrs with arity 0
Registering primOp __listToAttrs with arity 0
Registering primOp __intersectAttrs with arity 0
Registering primOp __catAttrs with arity 0
Registering primOp __functionArgs with arity 0
Registering primOp __mapAttrs with arity 0
Registering primOp __isList with arity 0
Registering primOp __elemAt with arity 0
Registering primOp __head with arity 0
Registering primOp __tail with arity 0
Registering primOp map with arity 0
Registering primOp __filter with arity 0
Registering primOp __elem with arity 0
Registering primOp __concatLists with arity 0
Registering primOp __length with arity 0
Registering primOp __foldl' with arity 0
Registering primOp __any with arity 0
Registering primOp __all with arity 0
Registering primOp __genList with arity 0
Registering primOp __sort with arity 0
Registering primOp __partition with arity 0
Registering primOp __concatMap with arity 0
Registering primOp __add with arity 0
Registering primOp __sub with arity 0
Registering primOp __mul with arity 0
Registering primOp __div with arity 0
Registering primOp __bitAnd with arity 0
Registering primOp __bitOr with arity 0
Registering primOp __bitXor with arity 0
Registering primOp __lessThan with arity 0
Registering primOp toString with arity 0
Registering primOp __substring with arity 0
Registering primOp __stringLength with arity 0
Registering primOp __hashString with arity 0
Registering primOp __match with arity 0
Registering primOp __split with arity 0
Registering primOp __concatStringsSep with arity 0
Registering primOp __replaceStrings with arity 0
Registering primOp __parseDrvName with arity 0
Registering primOp __compareVersions with arity 0
Registering primOp __splitVersion with arity 0
Registering primOp __unsafeDiscardStringContext with arity 1
Registering primOp __hasContext with arity 1
Registering primOp __unsafeDiscardOutputDependency with arity 1
Registering primOp __getContext with arity 1
Registering primOp __appendContext with arity 2
Registering primOp fetchMercurial with arity 1
Registering primOp fetchTree with arity 1
Registering primOp __fetchurl with arity 0
Registering primOp fetchTarball with arity 0
Registering primOp fetchGit with arity 0
Registering primOp fromTOML with arity 1
[       OK ] TrivialExpression.null (1 ms)
[ RUN      ] TrivialExpression.1
Registering primOp __getFlake with arity 1
Registering primOp scopedImport with arity 2
Registering primOp import with arity 0
Registering primOp __typeOf with arity 0
Registering primOp isNull with arity 0
Registering primOp __isFunction with arity 0
Registering primOp __isInt with arity 0
Registering primOp __isFloat with arity 0
Registering primOp __isString with arity 0
Registering primOp __isBool with arity 0
Registering primOp __isPath with arity 0
Registering primOp __genericClosure with arity 1
Registering primOp abort with arity 0
Registering primOp throw with arity 0
Registering primOp __addErrorContext with arity 2
Registering primOp __ceil with arity 0
Registering primOp __floor with arity 0
Registering primOp __tryEval with arity 0
Registering primOp __getEnv with arity 0
Registering primOp __seq with arity 0
Registering primOp __deepSeq with arity 0
Registering primOp __trace with arity 0
Registering primOp derivationStrict with arity 1
Registering primOp placeholder with arity 0
Registering primOp __toPath with arity 0
Registering primOp __storePath with arity 0
Registering primOp __pathExists with arity 0
Registering primOp baseNameOf with arity 0
Registering primOp dirOf with arity 0
Registering primOp __readFile with arity 0
Registering primOp __findFile with arity 2
Registering primOp __hashFile with arity 0
Registering primOp __readDir with arity 0
Registering primOp __toXML with arity 0
Registering primOp __toJSON with arity 0
Registering primOp __fromJSON with arity 0
Registering primOp __toFile with arity 0
Registering primOp __filterSource with arity 0
Registering primOp __path with arity 0
Registering primOp __attrNames with arity 0
Registering primOp __attrValues with arity 0
Registering primOp __getAttr with arity 0
Registering primOp __unsafeGetAttrPos with arity 2
Registering primOp __hasAttr with arity 0
Registering primOp __isAttrs with arity 0
Registering primOp removeAttrs with arity 0
Registering primOp __listToAttrs with arity 0
Registering primOp __intersectAttrs with arity 0
Registering primOp __catAttrs with arity 0
Registering primOp __functionArgs with arity 0
Registering primOp __mapAttrs with arity 0
Registering primOp __isList with arity 0
Registering primOp __elemAt with arity 0
Registering primOp __head with arity 0
Registering primOp __tail with arity 0
Registering primOp map with arity 0
Registering primOp __filter with arity 0
Registering primOp __elem with arity 0
Registering primOp __concatLists with arity 0
Registering primOp __length with arity 0
Registering primOp __foldl' with arity 0
Registering primOp __any with arity 0
Registering primOp __all with arity 0
Registering primOp __genList with arity 0
Registering primOp __sort with arity 0
Registering primOp __partition with arity 0
Registering primOp __concatMap with arity 0
Registering primOp __add with arity 0
Registering primOp __sub with arity 0
Registering primOp __mul with arity 0
Registering primOp __div with arity 0
Registering primOp __bitAnd with arity 0
Registering primOp __bitOr with arity 0
Registering primOp __bitXor with arity 0
Registering primOp __lessThan with arity 0
Registering primOp toString with arity 0
Registering primOp __substring with arity 0
Registering primOp __stringLength with arity 0
Registering primOp __hashString with arity 0
Registering primOp __match with arity 0
Registering primOp __split with arity 0
Registering primOp __concatStringsSep with arity 0
Registering primOp __replaceStrings with arity 0
Registering primOp __parseDrvName with arity 0
Registering primOp __compareVersions with arity 0
Registering primOp __splitVersion with arity 0
Registering primOp __unsafeDiscardStringContext with arity 1
Registering primOp __hasContext with arity 1
Registering primOp __unsafeDiscardOutputDependency with arity 1
Registering primOp __getContext with arity 1
Registering primOp __appendContext with arity 2
Registering primOp fetchMercurial with arity 1
Registering primOp fetchTree with arity 1
Registering primOp __fetchurl with arity 0
Registering primOp fetchTarball with arity 0
Registering primOp fetchGit with arity 0
Registering primOp fromTOML with arity 1
[       OK ] TrivialExpression.1 (0 ms)
[ RUN      ] TrivialExpression.minus1
Registering primOp __getFlake with arity 1
Registering primOp scopedImport with arity 2
Registering primOp import with arity 0
Registering primOp __typeOf with arity 0
Registering primOp isNull with arity 0
Registering primOp __isFunction with arity 0
Registering primOp __isInt with arity 0
Registering primOp __isFloat with arity 0
Registering primOp __isString with arity 0
Registering primOp __isBool with arity 0
Registering primOp __isPath with arity 0
Registering primOp __genericClosure with arity 1
Registering primOp abort with arity 0
Registering primOp throw with arity 0
Registering primOp __addErrorContext with arity 2
Registering primOp __ceil with arity 0
Registering primOp __floor with arity 0
Registering primOp __tryEval with arity 0
Registering primOp __getEnv with arity 0
Registering primOp __seq with arity 0
Registering primOp __deepSeq with arity 0
Registering primOp __trace with arity 0
Registering primOp derivationStrict with arity 1
Registering primOp placeholder with arity 0
Registering primOp __toPath with arity 0
Registering primOp __storePath with arity 0
Registering primOp __pathExists with arity 0
Registering primOp baseNameOf with arity 0
Registering primOp dirOf with arity 0
Registering primOp __readFile with arity 0
Registering primOp __findFile with arity 2
Registering primOp __hashFile with arity 0
Registering primOp __readDir with arity 0
Registering primOp __toXML with arity 0
Registering primOp __toJSON with arity 0
Registering primOp __fromJSON with arity 0
Registering primOp __toFile with arity 0
Registering primOp __filterSource with arity 0
Registering primOp __path with arity 0
Registering primOp __attrNames with arity 0
Registering primOp __attrValues with arity 0
Registering primOp __getAttr with arity 0
Registering primOp __unsafeGetAttrPos with arity 2
Registering primOp __hasAttr with arity 0
Registering primOp __isAttrs with arity 0
Registering primOp removeAttrs with arity 0
Registering primOp __listToAttrs with arity 0
Registering primOp __intersectAttrs with arity 0
Registering primOp __catAttrs with arity 0
Registering primOp __functionArgs with arity 0
Registering primOp __mapAttrs with arity 0
Registering primOp __isList with arity 0
Registering primOp __elemAt with arity 0
Registering primOp __head with arity 0
Registering primOp __tail with arity 0
Registering primOp map with arity 0
Registering primOp __filter with arity 0
Registering primOp __elem with arity 0
Registering primOp __concatLists with arity 0
Registering primOp __length with arity 0
Registering primOp __foldl' with arity 0
Registering primOp __any with arity 0
Registering primOp __all with arity 0
Registering primOp __genList with arity 0
Registering primOp __sort with arity 0
Registering primOp __partition with arity 0
Registering primOp __concatMap with arity 0
Registering primOp __add with arity 0
Registering primOp __sub with arity 0
Registering primOp __mul with arity 0
Registering primOp __div with arity 0
Registering primOp __bitAnd with arity 0
Registering primOp __bitOr with arity 0
Registering primOp __bitXor with arity 0
Registering primOp __lessThan with arity 0
Registering primOp toString with arity 0
Registering primOp __substring with arity 0
Registering primOp __stringLength with arity 0
Registering primOp __hashString with arity 0
Registering primOp __match with arity 0
Registering primOp __split with arity 0
Registering primOp __concatStringsSep with arity 0
Registering primOp __replaceStrings with arity 0
Registering primOp __parseDrvName with arity 0
Registering primOp __compareVersions with arity 0
Registering primOp __splitVersion with arity 0
Registering primOp __unsafeDiscardStringContext with arity 1
Registering primOp __hasContext with arity 1
Registering primOp __unsafeDiscardOutputDependency with arity 1
Registering primOp __getContext with arity 1
Registering primOp __appendContext with arity 2
Registering primOp fetchMercurial with arity 1
Registering primOp fetchTree with arity 1
Registering primOp __fetchurl with arity 0
Registering primOp fetchTarball with arity 0
Registering primOp fetchGit with arity 0
Registering primOp fromTOML with arity 1

andir · 2021-11-06T19:02:34Z

As it turned out, the arguments were moved out of the registration structure and thus every initialization after would get the wrong data. If we really care about that small amount of memory overhead, we should put them into a shared_ptr that is cheap to clone. As far as I can tell the evaluator only needs the structure to figure out how many arguments should there be. There is a one time operation. It is probably more of an impact on the new repl :doc feature, but a pointer indirection sounds like a reasonable tradeoff there. The simplest fix was to remove the std::move invocation. As done in
9bfc5d2.

andir · 2021-11-08T12:50:00Z

@regnat mind giving this another review?

edolstra · 2021-11-08T13:02:57Z

Note that we already have libexpr tests in tests/lang. What's the advantage of testing the evaluator in C++?

andir · 2021-11-08T13:42:23Z

Note that we already have libexpr tests in tests/lang. What's the advantage of testing the evaluator in C++?

I wanted some simple & fast tests that ensure that some basic assumptions around the language continue to hold true. It also serves as a way to ensure that we can eval in multiple separate "environments" (multiple EvalStates) during the runtime of a single process.

The tests currently execut within ~20ms and compile within a couple of seconds. I don't need a full (re)build of Nix executables to verify my changes aren't breaking fundamental assumptions.

I don't think this will replace the tests we have intests/lang but might be an option for more leightweight language feature tests. For example we can ensure that certain constructs are lazy (map) or that other operations. It is also easier (at least for me) to debug segfaults during eval when I can reproduce them in a minimal executable.

The tests/lang tests are great for larger expressions that might have failed in the past and that should not fail ever again. These smaller tests are more suited towards testing axioms and/or edge cases.

In the past made several attempts at refactoring various parts in libexpr and then ended up with some segfault that would only occur when testing against nixpkgs. Stepping through the eval of nixpkgs isn't really fun in my opinion. The hope is that we could reproduce certain issues with smaller and easier to debug unit tests in those cases.

gilligan · 2021-11-26T22:51:46Z

src/libexpr/tests/json.cc

+        mkString(v, "test");
+        ASSERT_EQ(getJSONValue(v), "\"test\"");
+    }
+


Maybe you could also add a test for unicode strings here - and as for "how is this even relevant" I would probably just say: it helps to stick to the same behaviour 🤷

andir · 2021-12-15T22:25:07Z

Rebased one more time and added another corner case that is odd in our current language implementation (or = 1; is valid but you can't ever access the value bound to it unless you go through something like x.or).

The example was previously indicating that multiple whitespaces would be collapsed into a single captured whitespace. That isn't true and was likely a mistake when being documented initially.

Since lists are just chunks of memory the individual elements in the list might be unitilized when a programming error happens within Nix. In this case the values are null-initialized (at least with Boehm GC) and we can avoid a nullptr deref when printing them. I ran into this issue while ensuring that new expression tests would show the actual value on an assertion failure. This is unlikely to cause any runtime performance regressions as printing values is not really in the hot path (unless the repl is the primary use case).

This introduces tests for libexpr that evalulate various trivial Nix language expressions and primop invocations that should be good smoke tests wheter or not the implementation is behaving as expected.

thufschmitt

@andir Sorry for taking so long to get back to it.

I’ve rebased the branch on top of master. I let you double-check that I didn’t screw anything up in the process if you want, and I think we can merge it afterwards

andir · 2022-05-06T14:48:56Z

@andir Sorry for taking so long to get back to it.

I’ve rebased the branch on top of master. I let you double-check that I didn’t screw anything up in the process if you want, and I think we can merge it afterwards

I did a brief casual browsing through the diff and I couldn't see anything that is obviously wrong. Go ahead and merge :-)

edolstra · 2022-05-10T08:48:19Z

I'm not very happy that we now have two different test suites for the evaluator. This just makes it confusing whether tests should go into libexpr-tests or tests/lang. If this PR had replaced tests/lang with libexpr-tests, that would have been one thing, but now it just increases maintainer burden.

edolstra · 2022-05-10T08:51:32Z

The other problem is that libexpr-tests is intimately tied to internal APIs, such as the Value representation. So changes to those internal interfaces (e.g. Value::listElems(), to give an example of something that has changed recently) would require a huge amount of work to update the tests - work that would be unnecessary with the test framework in tests/lang.

edolstra · 2022-05-10T08:55:34Z

This style of writing tests is also extremely verbose. Take this one:


    TEST_F(PrimOpTest, partition) {
        auto v = eval("builtins.partition (x: x > 10) [1 23 9 3 42]");
        ASSERT_THAT(v, IsAttrsOfSize(2));

        auto right = v.attrs->get(createSymbol("right"));
        ASSERT_NE(right, nullptr);
        ASSERT_THAT(*right->value, IsListOfSize(2));
        ASSERT_THAT(*right->value->listElems()[0], IsIntEq(23));
        ASSERT_THAT(*right->value->listElems()[1], IsIntEq(42));

        auto wrong = v.attrs->get(createSymbol("wrong"));
        ASSERT_NE(wrong, nullptr);
        ASSERT_EQ(wrong->value->type(), nList);
        ASSERT_EQ(wrong->value->listSize(), 3);
        ASSERT_THAT(*wrong->value, IsListOfSize(3));
        ASSERT_THAT(*wrong->value->listElems()[0], IsIntEq(1));
        ASSERT_THAT(*wrong->value->listElems()[1], IsIntEq(9));
        ASSERT_THAT(*wrong->value->listElems()[2], IsIntEq(3));
    }

This is a very verbose way of saying that the result should be equal to { right = [23 42]; left = [ 1 9 3 ]; }.

thufschmitt · 2022-05-10T09:31:26Z

If this PR had replaced tests/lang with libexpr-tests, that would have been one thing, but now it just increases maintainer burden.

A bit yes. But at the same time I wouldn’t want to commit to it too quickly (esp. wrt the dependency on the internal APIs that you mention: a priori, I think it’s a worthwile tradeoff, but I’d rather give it some time too ensure that it doesn’t cause too much pain before making a hard-to-revert change). Keeping it in addition to the existing testsuite makes it so that we have time to see how that’s gonna work out and migrate incrementally what we think needs to be migrated.

This style of writing tests is also extremely verbose

Yes, it is :) Maybe we could just compare the two values for this kind of more complex examples (either print the result and compare the strings, or use the equality operator from Nix).

This just makes it confusing whether tests should go into libexpr-tests or tests/lang

Yup’ this should have been settled on and documented more clearly. My take on that is that we should try to write new tests in libexpr-tests (both to make the experiment meaningful and because despite the verbosity overhead writing the tests here is so much faster). I’ll open a PR for that, letting you judge whether that seems a good plan.

nixos-discourse · 2022-05-12T13:46:48Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-30/19112/1

thufschmitt suggested changes Oct 13, 2021

View reviewed changes

andir force-pushed the libexpr-tests branch from 22b6968 to 0a94b5e Compare November 6, 2021 16:40

andir force-pushed the libexpr-tests branch 2 times, most recently from 82356ae to f39a988 Compare November 7, 2021 16:50

andir mentioned this pull request Nov 7, 2021

__functor causes infinite loop when it returns itself #5515

Closed

andir marked this pull request as ready for review November 8, 2021 12:49

andir force-pushed the libexpr-tests branch 2 times, most recently from 3b1008d to e15376f Compare November 26, 2021 22:24

gilligan reviewed Nov 26, 2021

View reviewed changes

andir force-pushed the libexpr-tests branch from e15376f to e15d746 Compare November 28, 2021 01:06

andir mentioned this pull request Nov 28, 2021

Don't move the arguments of the primOp #5680

Merged

andir force-pushed the libexpr-tests branch 3 times, most recently from d6d0f80 to a8c3e65 Compare November 28, 2021 18:48

andir force-pushed the libexpr-tests branch from a8c3e65 to 55d86bc Compare December 15, 2021 22:20

andir added 4 commits May 5, 2022 18:00

libexpr: fix builtins.split example

99668e5

The example was previously indicating that multiple whitespaces would be collapsed into a single captured whitespace. That isn't true and was likely a mistake when being documented initially.

Add operator<< for ValueTypes

cc05293

Add libexpr tests

73f4de1

This introduces tests for libexpr that evalulate various trivial Nix language expressions and primop invocations that should be good smoke tests wheter or not the implementation is behaving as expected.

thufschmitt force-pushed the libexpr-tests branch from 55d86bc to 73f4de1 Compare May 6, 2022 09:02

thufschmitt approved these changes May 6, 2022

View reviewed changes

thufschmitt merged commit 059ae7f into NixOS:master May 6, 2022

andir deleted the libexpr-tests branch May 7, 2022 11:53

thufschmitt mentioned this pull request May 12, 2022

Expand the testing section of the hacking docs #6517

Merged

Uh oh!

Conversation

andir commented Oct 12, 2021

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

andir Nov 6, 2021

Choose a reason for hiding this comment

Uh oh!

andir Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thufschmitt commented Oct 13, 2021

Uh oh!

andir commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vobloeb commented Oct 13, 2021

Uh oh!

andir commented Oct 13, 2021

Uh oh!

vobloeb commented Oct 13, 2021

Uh oh!

andir commented Nov 6, 2021

Uh oh!

andir commented Nov 6, 2021

Uh oh!

andir commented Nov 8, 2021

Uh oh!

edolstra commented Nov 8, 2021

Uh oh!

andir commented Nov 8, 2021

Uh oh!

gilligan Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andir commented Dec 15, 2021

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

andir commented May 6, 2022

Uh oh!

edolstra commented May 10, 2022

Uh oh!

edolstra commented May 10, 2022

Uh oh!

edolstra commented May 10, 2022

Uh oh!

thufschmitt commented May 10, 2022

Uh oh!

nixos-discourse commented May 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andir commented Oct 13, 2021 •

edited

Loading

gilligan Nov 26, 2021 •

edited

Loading