-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new C unit testing with utest.h library #53
base: main
Are you sure you want to change the base?
Conversation
This is now in a state that it is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An update to the README explaining how run these tests would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, I'm Alex. I'm an RSE working with @ma595 at the ICCS in Cambridge. I've been asked to provide a review for this PR.
It's looking good. A nice basis for building a test set / good proof of concept to get going with this. I've made some recommendations, mostly around naming conventions and comments.
As always, this is your repo. Please push back if there is a design decision for not following my recommendations & feel free to drop/resolve issues at your leisure (and if dropping, leave a brief justification for doing so below the comment so I don't need to follow-up) and ask for a re-review once comments have been addressed.
Happy Coding!
.github/workflows/c.yml
Outdated
run: | | ||
cd tests/C | ||
make | ||
./test_01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although there may be other ways of doing it, if a test name format is agreed e.g. test_ then running ./test_* or creating the list of tests and running over it may be a good thing to include in this change (or in it's own isolated change later) to aid traceability (rather than in a non-test framework specific change i.e. when you next add a test)
This would require that the source files had a different scheme for naming, see comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed using test*
tests/C/test_01.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend this is renamed in two ways:
First let's remove the prefix 'test' from test source files. If you prefer to indicate that they are a test source file in the filename use a simple pre-fix like 't'. So test_01 becomes t01.
Second, let's state the intent of the test in the filename to ensure maintainability in the future e.g. let's not say it's test_01 but test_filenameValidity (or similar). So, for example, t01.c becomes tFilenameValidity.c
This has some advantages in that it makes it simple to differentiate between source files and test source files (via the 't' prefix) but also enables differentiation with test source and text executable files (see comment above in c.yml). This distinction makes it simple to execute tests and the renaming can help make it clear what features are and aren't being tested at a glance.
This is just an example, please use a naming convention that suits you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed test_01.c to tMeteo_proc-dataset.c. The binary is now test_meteo_proc-dataset
tests/C/test_01.c
Outdated
@@ -0,0 +1,51 @@ | |||
/* | |||
test_01.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend this comment is updated to explain which features/code sections/functionality is being tested by this file.
I also recommend a line is added stating the modification date.
tests/C/test_01.c
Outdated
char *era_files_path = NULL; /* mandatory */ | ||
char *output_files_path = NULL; /* mandatory */ | ||
|
||
UTEST(test_is_valid_era_filename, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend the test objective of this test is stated in a comment above each unit test. It could be docstring formatted if the repo is setup for documentation generation or is intended to be.
A test objective should explain what the likely failure point is or how it might come about (perhaps explaining the series of asserts/how they are related). If there is a specific file or class/object which this test specifically covers, it would be useful to note that here. I appreciate some of this information is included in the name of the test but more info will be useful when a failure occurs. Some useful things to also include are associated Issue numbers for tests that verify a bug has been fixed.
tests/C/test_01.c
Outdated
|
||
UTEST_STATE(); | ||
|
||
int main(int argc, const char *const argv[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick inspection of the uTest repo, I agree with the comment that this may provide better functionality but if the extra functionality is not being used in this test, I recommend we swap to UTEST_MAIN in this file.
However I do appreciate this file may be acting as an example/template --- An alternative, if desired is to create a .md file for how to structure a test could be created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cool that you've wrapped this. I think another comment at the top of the file, explaining the intent/scope/limitations/motivations, would be useful and/or an entry in a doc page or a .md in the repo
At a later stage it may be worth creating a test that targets FAIL_UNLESS which verifies that common messages can be thrown, particularly if you create a set of standard error messages. It may perhaps be overkill with this change as any negative test will provide coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ARibecaJob I've noticed a bug in the macro. The "%s" string is not parsed from the message as done in the original CK_ASSERT_MSG function, meaning we lose printf style printing. It's possible to reproduce this by causing one of the tests to fail.
I've corrected it to have the "Message", "Parameter" syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Matt,
sorry for the late reply.
You're right about the bug but I want to propose another solution because your fix has quirks when the arguments passed to the function aren't two (message and parameter)
ASSERT_MSG(6 == 5, "6 == 5");
become
Message : 6 == 5, parameter: (null)
and
ASSERT_MSG(0, "False should be returned for this string: '%s' '%s' '%s'", "filename.ext", "one", "two");
became
Message : False should be returned for this string: '%s' '%s' '%s', parameter: filename.ext
so I proposed this new define that it should works for any case:
#define ASSERT_MSG(x,...) \
UTEST_SURPRESS_WARNING_BEGIN do { \
const int xEval = !!(x); \
if (!(xEval)) { \
UTEST_PRINTF("%s:%i: Failure\n", __FILE__, __LINE__); \
UTEST_PRINTF(" Condition : (%s)\n", #x); \
UTEST_PRINTF("%s", " Message : "); \
UTEST_PRINTF(__VA_ARGS__); \
UTEST_PRINTF("%s", "\n"); \
*utest_result = UTEST_TEST_FAILURE; \
return; \
} \
} \
while (0) \
UTEST_SURPRESS_WARNING_END
and finally we have
Message : 6 == 5
Message : False should be returned for this string: 'filename.ext' 'one' 'two'
Let me know if it works for you.
You can change your PR with this.
tests/C/utest_oneflux.h
Outdated
UTEST_SURPRESS_WARNING_END | ||
|
||
|
||
#define CK_ASSERT_MSG FAIL_UNLESS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest this gets renamed to CHECK_ASSERT_MSG or simply CHECK_ASSERT unless there is a name clash (I don't think CK is universal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to ASSERT_MSG
Hi, first of all thank you for your time. I think we should decide whether to focus on the writing speed of the tests or on the verbosity for a better understanding: |
Hi @ARibecaJob, could you take a look at the changes I've made to this PR? In particular the change to |
Just a bump @ARibecaJob to check @ma595's changes. It would be great to see this merged. |
Add C testing framework using utest and production function from meteo_proc code:
Makefile
used to compile tests
main.c
based on @ma595 code, please see PR [WIP] Python testing framework and workflow #45
utest.h
unit test library from https://github.com/sheredom/utest.h
utest_oneflux.h
contains custom functions that extend utest.h library
and mimic function from check library (https://libcheck.github.io/check/)