Improved testing, property testing, device tests

[tsturzl]

The first part of this PR addresses part of #8 by adding tests to the cgroup device v1 controller. This PR continues on to improve testing by adding traits to for generating inputs for property testing to a few oci-spec types, and subsequently using the traits to implement property testing for the following cgroup v1 controllers:

• Devices
• Hugetbl
• Memory

A few more subsystems could benefit from this type of testing, but I don't believe it's worth the effort to implement it for every controller. The oci-spec traits are only available with a certain feature flag proptesting, to prevent the quickcheck dependency and the trait implementations from being included in regular builds.

Property testing should give us a much better chance of catching edge cases. It creates fewer tests which test a multitude of generated inputs, meaning we need fewer tests to catch the same issues. It also arguably provides many of the benefits of fuzzing, which ultimately aligns with the objectives of runtime safety. I feel like this is an excellent choice for areas with more complicated logic, not just writing values to files for example.

Let me know what you think, and if it's agreed that this is a good approach moving forward I will create another issue listing other areas where we could benefit from property testing.

[tsturzl]

I'm asking for 2 reviewers @utam0k and @Furisto to get input on testing moving forward. I think both your input is valuable here in determining whether or not this is a good approach, and generally getting a discussion going with both of you on testing moving forward.

[utam0k]

This PR is superb. The changes in this PR are a bit big, so I don't think I'll be able to review it right away, so I'll try to find time to review it thoroughly.
I'd also like to have @Furisto review it as well.

[utam0k]

Suggested change

	.fold(true, |a, b| a && b)
	.all(|is_ok| is_ok)

[tsturzl]

Good catch, I was actually looking for this exactly. Much cleaner and more idiomatic.

[tsturzl]

this change has been made in the recent commit

[utam0k]

I think this change is a good thing. But have there actually been any such a case?

[tsturzl]

I think it matches the test case better, but either way the tests pass. I think it might actually be useful here to see what the subsystem thinks of negative values. I'm not sure if the kernel will give a validation error for negative values less than -1 or if all negative values are considered unlimited. If it's the case that all negative values are unlimited then I would say this changed behavior is most correct, otherwise I'd say any negative value less than -1 should cause it's own expected failure and the tests should account for that. I'll look into this deeper, and leave a comment in the code to explain.

[tsturzl]

I decided to revert this. So if you set any of these below -1 it will result in an "Invalid Arguments" error, which will be the same thing that happens in Rust. We don't currently deal with any of the validation error's the kernel spits out, so I decided to just leave it. Eventually it might be nice to handle the error in the controller code and create our own set of errors to more reasonably represent what the error actually means, and provide some feedback of what the valid values actually are.

[utam0k]

I felt it easier to read the read_to_string import in the test scope.

[tsturzl]

I started thinking the same thing. At first it was only used a few times, but now it's getting pretty verbose.

[tsturzl]

I am now importing read_to_string in to all of the tests I had touched with this PR.

[utam0k]

@tsturzl
I added some comments. However, the whole direction is very cool.

[tsturzl]

@utam0k Excellent feedback! I'll take a further look into this hopefully tomorrow.

[Furisto]

@tsturzl @utam0k
Thank you for this. I think its an interesting and cool approach. My concern is that it seems to encourage big complicated tests, which check for everything instead of small focused ones. I do not have practical experience with property based testing though and maybe this concern is unfounded. I can certainly see the appeal and am open to trying it out.

[tsturzl]

@Furisto I don't think we'll want to stop writing smaller focused unit tests, and property testing isn't really useful everywhere. Especially in the case of some of these cgroup v1 controllers which are basically writing values to files. My concern comes when we start testing things like the Memory, Hugetbl, Blkio controllers which have more complicated behavior. I think we still keep the same method of testing, but simply add property testing where necessary. If it becomes a burden to keep up with or the test cases fail to provide utility it should be separated enough to easily pull out. I don't think currently we have broad enough tests cases, and writing out a bunch of static test data to cover every case would probably exceed the efforts of property testing to provide a far lesser depth of inputs. So admittedly this appeals to my laziness. In many cases having explicit testing for specific behavior is superior, but I say we still write those tests. So I see this more as a supplement to current means of testing rather than a replacement for it, this is why I still wrote some basic tests for the Devices Controller and left the previously written tests in place.

[utam0k]

nice improvements!