new C unit testing with utest.h library

[ARibecaJob]

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 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/)

[mondus]

This is now in a state that it is ready for review.

[dorchard]

An update to the README explaining how run these tests would be great.

[a-smith-github]

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!

[a-smith-github]

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

[ma595]

fixed using test*

[a-smith-github]

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.

[ma595]

Changed test_01.c to tMeteo_proc-dataset.c. The binary is now test_meteo_proc-dataset

[a-smith-github]

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.

[a-smith-github]

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.

[a-smith-github]

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.

[a-smith-github]

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

[ma595]

@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.

[ARibecaJob]

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.

[a-smith-github]

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).

[ma595]

Renamed to ASSERT_MSG

[ARibecaJob]

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!

Hi,

first of all thank you for your time.
I want to take advantage of this reply to try to have guidelines with the other members

I think we should decide whether to focus on the writing speed of the tests or on the verbosity for a better understanding:
it's an open source project but you can agreed that it's better to spend time coding new features (which obviously will lead to writing new tests!) that write tests
Despite this, I'd rather be verbose about renaming files and have the prefix 'test_' instead of the single 't'
but since these are already in a folder called "Test", perhaps we could completely remove the prefixes and adopt the single t, for example, 't01.c'
It's better to create test suites within a single file instead of having individual files for each function to test ?
We may group functions based on their scope e.g. a test file 'input.c' that has all functions related to import\check files.

[ma595]

Hi @ARibecaJob, could you take a look at the changes I've made to this PR? In particular the change to tests/C/utest_oneflux.h

[dorchard]

Just a bump @ARibecaJob to check @ma595's changes. It would be great to see this merged.