[fontc] More code reuse in fontc tests #584
Conversation
|
Also as a general observation (not having really looked at this in much detail before) a bunch(/most/all?) of the tests in |
| /// | ||
| /// When you need more control, just the `compile` fn/`TestCompile` type | ||
| /// directly | ||
| struct SimpleCompile { |
There was a problem hiding this comment.
I like the elimination of boilerplate but I'm not sure why you need a new type, just improve TestCompile?
There was a problem hiding this comment.
Filed as #594 to unblock merging
|
I looked into that, and it isn't obvious how to present this sort of simple interface there, while preserving the flexibility of the existing type. This solution doesn't feel great, but it does feel like a small improvement. |
| fn new(source: &str) -> std::io::Result<Self> { | ||
| let temp_dir = tempdir()?; | ||
| let build_dir = temp_dir.path(); | ||
| let result = compile(Args::for_test(build_dir, source)); |
There was a problem hiding this comment.
I don't particularly like that the heavy-lifting part of the code where things get compiled is hidden in a default new() constructor.. Why not use a free function that takes a str and returns a struct and is named with something like "compile" to make it clear this is, well, compiling?
There was a problem hiding this comment.
+1, I like the current result = compile() pattern. I also like how this PR removes some of the boilerplate. Seems possible to combine.
There was a problem hiding this comment.
fair, how about SimpleCompile::compile?
cmyr
force-pushed
the
test-setup-reuse
branch
from
0d1e3a9 to
9088471
Compare
cmyr
force-pushed
the
test-setup-reuse
branch
from
9088471 to
d3b2076
Compare
|
okay I'm inclined to get this in, it isn't a silver bullet but it marginally improves the status quo |
In the process of looking into how to add mark-mark tests I got a bit distracted trying to figure out a way to reduce the boilerplate in the existing tests.
This mostly does that, although it also slightly tweaks how some asserts are structured.