search

hapijs / lab

Node test utility
Other
739 stars 176 forks source link

New --coverage-predicates option #1043

Closed kanongil closed 2 years ago

kanongil commented 2 years ago

This new option is designed to help improve code coverage for platform specific features, by providing a way to optionally signal when coverage has to be disabled. It was inspired by the unconditional coverage exclusion in https://github.com/hapijs/lab/pull/1036/commits/cfbf0601749f963245f6cc00e61a89eb8e0d06a0 from #1036, added only because it is not covered when running on node v12.

I designed it as a powerful primitive, with a simple implementation footprint of about 10 lines of code. For #1036, it can enable coverage on node 14+, by adding a new has-nullish predicate in .labrc.js when running on node 14+, and revising the "off" line to: /*$lab:coverage:off$ $not:has-nullish$*/.

I considered allowing multiple extra predicates with an implicit AND condition, but it would add extra complexity to the implementation. Also, it is still possible for a user to combine multiple predicates, by creating new composite predicates, eg. node14+win32.

FYI, I have previously discarded a hapi feature I worked on, only because it could not work on Windows, and adding coverage bypasses would mean that I could not reliably reach 100% coverage on the other platforms. With this feature, it could have been completed.

Note, I haven't yet documented how this is used inside coverage bypass comments since the syntax might still change depending on feedback.

kanongil commented 2 years ago

Hmm, one thing that could make the feature safer, would be if the predicates are specified as an object with true / false. This could would allow lab to verify that all predicates are actually used, and that all embedded predicate values exist. However, this is less compatible with the CLI options syntax.

I guess it could work as type json, and it could be specified as --coverage-predicate.nullish true. Also, this feature will probably mostly be used from .labrc.js to compute the values based on the current runtime.

kanongil commented 2 years ago

Rebased, and changes CLI interface to match my previous comment. Note, that I did not add any extra verification at this time. However, it is now possible to add the functionality in a later patch.

kanongil commented 2 years ago

Thanks for the support @devinivy. I assume you approve of the new syntax, as an extra embed inside $$?

I considered different approaches, but ended up with this. I think it is pretty clear, while signalling to a reader that it is a special syntax that is not just a comment. I briefly considered doing it plain, without the $$ (eg. /* $lab:coverage:off$ not:has-nullish */), but it made it feel more like a regular comment. I also tried embedding it inside the existing $$, but that felt cluttered and hard to read.

kanongil commented 2 years ago

We might also consider exposing the predicates to the Lab.script() in test codes, so they can easily lookup these and apply them to eg. a test { skip }option.

devinivy commented 2 years ago

Yeah, I think the syntax is appropriate. I agree it would be nice to be able to resolve those as part of skips, but I also think we can handle that as an iterative improvement. One concept that comes to mind is to have a special value that can be resolved when running the tests, e.g.

{ skip: Lab.predicate('not:has-nullish') }
Nargonath commented 2 years ago

Thanks @kanongil, it definitely looks like a powerful feature. 💪 Well done.