Add memory location comparison macros (ck_assert_mem_*) #64

prusnak · 2016-10-19T18:59:38Z

As discussed here: https://sourceforge.net/p/check/feature-requests/30/

brarcher · 2016-10-20T01:17:40Z

Jenkins: ok to test
^ necessary to start the remaining of the tests

brarcher

Generally I like it. There needs to be a trade-off for how much is printed, as it cannot be boundless.

This change will need some unit tests in order to cover the new API. Can you also provide these unit tests along with your change?

brarcher · 2016-10-20T01:26:29Z

src/check.h.in

+/* The x and y parameter swap in memcmp() is needed to handle >, >=, <, <= operators */
+/* Output is limited to 64 bytes (or 128 hex digits) */
+#define _ck_assert_mem(X, OP, Y, L) do { \
+  const char* _ck_x = (const char*)(void*)(X); \


why is the (void*) cast necessary?

Just to be explicit, in case there is a platform where char!=8bits, should this be uint8_t?

Changed to uint8_t and removed void* cast.

brarcher · 2016-10-20T01:26:36Z

src/check.h.in

+  _ck_x_str[_ck_i * 2] = 0; \
+  _ck_y_str[_ck_i * 2] = 0; \
+  ck_assert_msg(0 OP memcmp(_ck_y, _ck_x, _ck_l), \
+    "Assertion '"#X#OP#Y"' failed: "#X"==\"%s\", "#Y"==\"%s\"", _ck_x_str, _ck_y_str); \


I am torn between the message containing "#X#OP#Y" or something else which mentions the size of the strings. Should the message mention memcmp, as the documentation uses? For example, "0#OPmemcmp(#X,#Y,#L)"?

I copied this from ck_assert_str usage, where strcmp is not mentioned in the output (but it is in the documentation). I would prefer not adding it to the output. However I am fine with adding length somewhere.

brarcher · 2016-10-20T01:29:52Z

src/check.h.in

+  const char* _ck_x = (const char*)(void*)(X); \
+  const char* _ck_y = (const char*)(void*)(Y); \
+  size_t _ck_l = (L); \
+  char _ck_x_str[129]; \


Can the magic numbers used in the macro, e.g. "129", "64", be defined? For example CK_MAX_ASSERT_MEM_PRINT_SIZE=64 which is then used everywhere?

Additionally, perhaps define it as such:

#ifndef CK_MAX_ASSERT_MEM_PRINT_SIZE #define CK_MAX_ASSERT_MEM_PRINT_SIZE #endif

That way if a user wishes to override it they may. Another option it to make it a configure option

brarcher · 2016-10-20T01:35:14Z

src/check.h.in

+  size_t _ck_l = (L); \
+  char _ck_x_str[129]; \
+  char _ck_y_str[129]; \
+  static char _ck_hexdigits[] = "0123456789abcdef"; \


Could this array be made const?

brarcher · 2016-10-20T01:36:49Z

src/check.h.in

+  char _ck_y_str[129]; \
+  static char _ck_hexdigits[] = "0123456789abcdef"; \
+  size_t _ck_i; \
+  for (_ck_i = 0; _ck_i < ((_ck_l > 64) ? 64 : _ck_l); _ck_i++) { \


can the computation for the string size be done before the loop, for readability?

prusnak · 2016-10-20T08:25:07Z

Pushed a new commit fixing the issues from above.

prusnak · 2016-10-20T09:55:29Z

Also I am adding the following for discussion. The way I usually use these macros is like this:

ck_assert_mem_eq(der, fromhex("3007020166020200ee"), 9);

while the function fromhex is defined like this:

#define FROMHEX_MAXLEN 256

const uint8_t *fromhex(const char *str)
{
    static uint8_t buf[FROMHEX_MAXLEN];
    size_t len = strlen(str) / 2;
    if (len > FROMHEX_MAXLEN) len = FROMHEX_MAXLEN;
    for (size_t i = 0; i < len; i++) {
        uint8_t c = 0;
        if (str[i * 2] >= '0' && str[i*2] <= '9') c += (str[i * 2] - '0') << 4;
        if ((str[i * 2] & ~0x20) >= 'A' && (str[i*2] & ~0x20) <= 'F') c += (10 + (str[i * 2] & ~0x20) - 'A') << 4;
        if (str[i * 2 + 1] >= '0' && str[i * 2 + 1] <= '9') c += (str[i * 2 + 1] - '0');
        if ((str[i * 2 + 1] & ~0x20) >= 'A' && (str[i * 2 + 1] & ~0x20) <= 'F') c += (10 + (str[i * 2 + 1] & ~0x20) - 'A');
        buf[i] = c;
    }
    return buf;
}

Do you think it makes sense to integrate it further in the check library, so we could have something like this?

ck_assert_mem_hex_eq(der, "3007020166020200ee", 9);

It is much more convenient (especially for longer values, eg. 64 bytes) than using:

ck_assert_mem_hex_eq(der, "\x30\x07\x02\x01\x66\x02\x02\x00\xee", 9);

brarcher · 2016-10-25T03:47:57Z

I can appreciate the use case you mention regarding the hex. What gives me pause is it would be another place where a maximum size would be used (for the static buffer). The first place using a max size, the strings which reports buffer contents on a failure, does not result in a problem if more data is used than the buffer size. In that case the differences in the two buffers will simply not be printed, but it will not affect the success or failure of the comparison. However, in the case of the hex based comparison call because it may not be obvious to a caller that the hex string provided has a maximum possible length, passing too many characters may affect the expected result. In addition, if an odd number of characters were passed the last character would be ignored.

From projects where I've compared binary data typically the expected data is in a file, and before the comparison the file's contents are loaded into a buffer. However, those typically are for comparisons that are larger than one would want to type for a unit test.

My opinion is to provide the ck_assert_mem_* calls for now, and if there is a way to quell the concerns listed above maybe a ck_assert_hex call can be added as a separate commit.

prusnak · 2016-10-25T08:16:54Z

Ok, let's focus on ck_assert_mem_* for now. Do you want some other changes or the commit is good as it is now?

brarcher · 2016-10-25T19:39:58Z

Do you want some other changes or the commit is good as it is now?

The change will need to have some unit tests accompany it. You can add some test cases under the test_ck_assert_ptr_ne test here:

https://github.com/libcheck/check/blob/master/tests/check_check_sub.c#L555

You will also need to add the test cases around here:

https://github.com/libcheck/check/blob/master/tests/check_check_sub.c#L1098

After that, you will need to determine the expected result of each of the test cases (there should be some tests that pass the check and some that fail) and put the expected result at the end of the pointer-based tests in the master_tests table:

https://github.com/libcheck/check/blob/master/tests/check_check_master.c#L117

Just keep the order of adding the tests to the simple test case and the order in the master_tests table consistent to avoid issues. Look around check_check_sub.c for examples of other tests for the ck_assert* macros for inspiration.

After adding those tests the change should be good to go. Let me know if you have any questions or run into something when adding the unit tests.

prusnak · 2016-10-26T13:27:00Z

Added unit tests as requested in f9676cf ...

These are basically ck_assert_str_* tests, but using binary data instead of strings.

brarcher · 2016-10-26T18:45:18Z

tests/check_check_sub.c

@@ -553,6 +553,78 @@ START_TEST(test_ck_assert_ptr_ne)
 }
 END_TEST

+START_TEST(test_ck_assert_mem_eq)


Because there is a limit on the number of characters printed in the assert message, there will need to be a test that check the boundary conditions of the message. Namely, a test comparing 0 bytes (should never produce a message), a test where all the bytes are printed (covered already), a test which is the max number of bytes allowed, and a test with too many bytes. Can you add tests for these cases?

brarcher · 2016-10-26T18:47:27Z

It would appear that one of the unit tests is not passing. This example shows the following message in the test run output:

/home/travis/build/libcheck/check/tests/check_check_master.c:473:F:Core Tests:test_check_all_msgs:56: Expected Assertion '", got Assertion '"\x00\x00\x00\x00\x01" == s' failed: "\x00\x00\x00\x00\x01" == "0000000001", s == "0000000002"

I believe that means there was no failure expected but a failure did occur. I am not sure the reason for this.

If you would like to reproduce this locally, there is a way to disable the unit tests which take a long time to run. Give the --disable-timeout-tests argument to configure and then the unit tests should run fairly quickly. You could also execute the check_check unit test program directly and check for a non-zero return code.

prusnak · 2016-10-27T12:42:50Z

I believe that means there was no failure expected but a failure did occur. I am not sure the reason for this.

Ah, there was an error in check_check_master.c. Should have used double backslash in C string.

Also added two new tests: test_ck_assert_mem_zerolen and test_ck_assert_mem_eq_long.

brarcher · 2016-10-28T23:44:08Z

tests/check_check_sub.c

+}
+END_TEST
+
+START_TEST(test_ck_assert_mem_expr)


what does this test for which the other test do not already test for?

brarcher · 2016-10-28T23:44:25Z

tests/check_check_sub.c

+  const char *t[] = { "\x00\x00\x00\x00\x01", "\x00\x00\x00\x00\x02" };
+  int i = -1;
+  ck_assert_str_eq(s, t[++i]);
+  ck_assert_str_eq(s, t[i]);


Are not both of these identical? Is not t[1] never used?

brarcher · 2016-10-28T23:45:43Z

tests/check_check_sub.c

+START_TEST(test_ck_assert_mem_eq_long)
+{
+  const char *s = "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02";
+  const char *t = "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01";


To easily check that the two arrays are both at least 65 characters long, might it be easier to:

const char s[65] = {0};
const char t[65] = {1};

Yes, but I need a different character at the end of the array, not at the beginning.

That is fine, it is OK as is.

brarcher · 2016-10-29T01:24:33Z

tests/check_check_master.c

+  { "Simple Tests", CK_FAILURE, "Assertion 's >= t' failed: s == \"0000000001\", t == \"0000000002\"" },
+  { "Simple Tests", CK_PASS,    "Passed" },
+  { "Simple Tests", CK_PASS,    "Passed" },
+  { "Simple Tests", CK_FAILURE, "Assertion 't == s' failed: t == \"00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\", s == \"00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\"" },


Here is a question about the output of an assertion when the data is too long. In this example we see that both the first and second argument look the same though the data is different. When the data is too long and it is truncated should a "..." be added to the end of the two arguments, so it is obvious to the developer that the difference may not be in the portion of the string shown?

Added in 8b1cd61

brarcher · 2016-10-31T02:28:34Z

src/check.h.in

@@ -772,6 +772,12 @@ CK_DLL_EXP void CK_EXPORT _ck_assert_failed(const char *file, int line,
  } \
  _ck_x_str[_ck_i * 2] = 0; \
  _ck_y_str[_ck_i * 2] = 0; \
+  if (_ck_i != _ck_l) { \


Would it not be more direct to compare _ck_l against _ck_maxl? It may be more intuitive.

brarcher · 2016-10-31T02:29:34Z

tests/check_check_master.c

@@ -121,7 +121,7 @@ static master_test_t master_tests[] = {
  { "Simple Tests", CK_FAILURE, "Assertion 't > t' failed: t == \"0000000002\", t == \"0000000002\"" },
  { "Simple Tests", CK_FAILURE, "Assertion 's >= t' failed: s == \"0000000001\", t == \"0000000002\"" },
  { "Simple Tests", CK_PASS,    "Passed" },
-  { "Simple Tests", CK_FAILURE, "Assertion 't == s' failed: t == \"00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\", s == \"00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\"" },
+  { "Simple Tests", CK_FAILURE, "Assertion 't == s' failed: t == \"000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000..\", s == \"000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000..\"" },


As there would be a difference between one check against CK_MAX_ASSERT_MEM_PRINT_SIZE characters and one with CK_MAX_ASSERT_MEM_PRINT_SIZE+1 characters, can there be a test using exactly CK_MAX_ASSERT_MEM_PRINT_SIZE characters to show that there is no .. at the end?

brarcher · 2016-11-01T02:41:42Z

I think that this change is good to go, thanks for the contribution.

Would you mind adding documentation for this change here, perhaps in a separate commit? Maybe add it after the description of ck_assert_ptr_eq and ck_assert_ptr_ne.

I'll look into the unit test which is failing. It appears to be failing before this change, so I believe it to be unrelated.

prusnak · 2016-11-01T12:08:18Z

Added documentation in a separate commit.

brarcher · 2016-11-03T15:25:05Z

Thanks for the contribution!

brarcher · 2016-11-05T00:34:13Z

Oh, I forgot to ask, @prusnak . If you are interested would you mind submitting a change for the AUTHORS file, adding an entry for yourself?

prusnak · 2016-11-05T11:12:10Z

Added in #68

brarcher reviewed Oct 20, 2016

View reviewed changes

Add memory location comparison macros (ck_assert_mem_*)

e8350b8

brarcher reviewed Oct 26, 2016

View reviewed changes

brarcher reviewed Oct 28, 2016

View reviewed changes

brarcher reviewed Oct 29, 2016

View reviewed changes

brarcher reviewed Oct 31, 2016

View reviewed changes

Add unit tests for memory location comparison macros (ck_assert_mem_*)

152577a

Add documentation for memory location comparison macros

6abda25

brarcher merged commit 9084971 into libcheck:master Nov 3, 2016

prusnak deleted the ck_assert_mem branch November 4, 2016 18:08

Add memory location comparison macros (ck_assert_mem_*) #64

Add memory location comparison macros (ck_assert_mem_*) #64

Conversation

prusnak commented Oct 19, 2016

brarcher commented Oct 20, 2016

brarcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusnak commented Oct 20, 2016

prusnak commented Oct 20, 2016 • edited Loading

brarcher commented Oct 25, 2016

prusnak commented Oct 25, 2016

brarcher commented Oct 25, 2016

prusnak commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

brarcher commented Oct 26, 2016

prusnak commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusnak Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brarcher commented Nov 1, 2016

prusnak commented Nov 1, 2016

brarcher commented Nov 3, 2016

brarcher commented Nov 5, 2016

prusnak commented Nov 5, 2016

prusnak commented Oct 20, 2016 •

edited

Loading

prusnak commented Oct 26, 2016 •

edited

Loading

prusnak Oct 29, 2016 •

edited

Loading