alistaire47
commented
2 years ago The only other thing that springs to mind right now is: should we also have a test for the roundtrip write results to disk, and then read them back in as if this were a new session | the results object wasn't coming directly from the benchmark running?
The existing usage does test a bunch of paths already because of how it works across processes (so when you screw something up here everything breaks 😅), but yeah, it's a good idea to have some isolated unit tests that just instantiate manually and test directly. I'll add test-result.R
Closes #99. Implements some more robust R6 classes in
result.Rto replace the old S3arrowbench_resultandarrowbench_results. The new classes can easily be serialized/deserialized to/from lists/strings of JSON/JSON files, and have active bindings for the various attributes so there's a clear place in the code to add validation for each that will get called regardless of how the instance is created (e.g. complete with$new(), deserialized from JSON, or overriding various bits later). This adds some boilerplate now, but means we don't need to rearrange anything to add validation later.The flow for the
as.data.frame()methods andget_params_summary()has been rearranged to call R6 methods, so you can call them directly, but existing code will continue to work exactly the same.This PR notably does not actually implement the validation it enables, given we're likely to evolve the schemas this defines soon. If we eventually want to make a nested
ResultMetadataorPackagesobject that lets us easily validate further down, the tools are in place though (e.g. recursive serialization should mostly just work).