On 06/01/2016 11:19 PM, David Malcolm wrote:
This is effectively v5 of the unittests proposal; for the earlier
versions see:
In general: nice to see this moving forward.
There are some issues with the patch kit, some patches seem to fix
issues with earlier ones (#3 or #13). One patch adds a sorry, the next
changes it to inform; patch #1 adds includes top toplev.c without adding
the included files. All these issues should be resolved: any patch
series should compile if it is applied only partially up to a point; no
early patch should depend on later ones. Also, bugfixes and behaviour
changes should be merged directly into the code so as to not hopelessly
confuse the reviewers.
Some of the cover letters, in particular #1 seem to contain outdated
information.
I also patched things so that it aborts on the first failure, making
failures easy to track down in the debugger, though requiring us to
make the selftests robust.
Is there any kind of doubt that this is a good requirement?
* conversion of the Levenshtein selftest from a plugin to using
-fself-test (we could move various other tests from plugins to run
earlier).
That sounds good.
Patch 4 shows a way of filtering the tests using a command-line regex.
What's the use case for this?
For (a), this version of the patch kit switches to stopping at the
first failure.
For (b), this version of the patch kit stays with auto-registration.
For (c), this version stays with a "lite" version of GTest.
Thoughts? Does the "running tests early" approach suggest answers to
these?
I think this is mostly a good resolution, although I have one particular
issue that rubs me the wrong way where I'd go even "liter" on the GTest.
In my view, tests are functions, and using object-orientation leads to
oddly contorted code. An example can be found in patch #5:
+class range_contains_point_tests : public ::selftest::test
+{
+ protected:
+ layout_range
+ make_range (int start_line, int start_col,
+ int end_line, int end_col)
(note also that this defeats the purpose of the GNU function formatting
which is to enable grep ^function_name to find them)
+ {
+ const expanded_location start_exploc
+ = {"test.c", start_line, start_col, NULL, false};
+ expanded_location finish_exploc
+ = {"test.c", end_line, end_col, NULL, false};
+
+ return layout_range (&start_exploc, &finish_exploc, false,
+ &start_exploc);
+ }
+};
I think I raised this before, but defining a class only to define
functions inside them seems relatively pointless; if anything you want
namespaces. This one doesn't even seem to contain any references to the
selftest framework so it could immediately be converted to a function.
Other such cases appear to use the EXPECT_EQ etc. macros, which call
pass and fail methods, but these just delegate immediately to the
runner's pass and fail. Which then raises the question whether runner
needs to be an object if there's only ever going to be one - once again
I think a functional rather than object-oriented style would be more
suitable to the problem. The main (only?) reason to have tests declared
as objects is for the auto-registration, beyond that it serves very
little purpose.
There are some inconsistent spellings of an end comment across the series:
+} // anon namespace
+} /* anon namespace. */
I wasn't sure we were doing these at all, but it turns out the former is
the canonical one (with a single space).
Bernd
