common: core_log_default_function() ut

[grom72]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.08%. Comparing base (ad17ad3) to head (3ee560e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6029      +/-   ##
==========================================
+ Coverage   70.05%   70.08%   +0.03%     
==========================================
  Files         133      133              
  Lines       19563    19563              
  Branches     3263     3263              
==========================================
+ Hits        13704    13711       +7     
+ Misses       5859     5852       -7     

[grom72]

Reviewed 11 of 16 files at r9, 5 of 5 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

[janekmi]

Reviewed 1 of 5 files at r6, 16 of 16 files at r9, 5 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 37 unresolved discussions (waiting on @grom72)

a discussion (no related file):
Please update the assumption comment:

Code snippet:

 * ASSUMPTIONS:
 * - level >= CORE_LOG_LEVEL_HARK && level <= CORE_LOG_LEVEL_DEBUG

---

---

src/test/core_log_default_function/core_log_default_function.c line 47 at r10 (raw file):

static struct {
	char *ret;
}Strchr_context;

Suggestion:

} Strchr_context;

---

src/test/core_log_default_function/core_log_default_function.c line 52 at r10 (raw file):

FUNC_MOCK_RUN_DEFAULT {
	UT_ASSERTeq(__c, '/');
	return Strchr_context.ret;

Not set anywhere explicitly?

Code quote:

return Strchr_context.ret;

---

src/test/core_log_default_function/core_log_default_function.c line 52 at r10 (raw file):

FUNC_MOCK_RUN_DEFAULT {
	UT_ASSERTeq(__c, '/');
	return Strchr_context.ret;

No fail case tested?

---

src/test/core_log_default_function/core_log_default_function.c line 60 at r10 (raw file):

	int line_no;
	const char *function_name;
	char *file_info_buffer;

Not set?

---

src/test/core_log_default_function/core_log_default_function.c line 61 at r10 (raw file):

	const char *function_name;
	char *file_info_buffer;
	int forceError;

No camel case, please.

Suggestion:

force_error

---

src/test/core_log_default_function/core_log_default_function.c line 62 at r10 (raw file):

	char *file_info_buffer;
	int forceError;
}Snprintf_context;

Suggestion:

} Snprintf_context;

---

src/test/core_log_default_function/core_log_default_function.c line 67 at r10 (raw file):

		const char *__restrict __format, ...)
/* file info */
	FUNC_MOCK_RUN(0) {

I believe all cases should be indented exactly the same as FUNC_MOCK_RUN_DEFAULT.

Suggestion:

FUNC_MOCK_RUN(0) {

---

src/test/core_log_default_function/core_log_default_function.c line 71 at r10 (raw file):

		va_list arg;
		va_start(arg, __format);
		UT_ASSERTstreq(__format, "%s: %3d: %s:  ");

Wow. Why it didn't have failed?

Suggestion:

"%s: %3d: %s: "

---

src/test/core_log_default_function/core_log_default_function.c line 78 at r10 (raw file):

			UT_ASSERTeq(line_no, Snprintf_context.line_no);
		else
			return -1;

You really don't have to do it. It is really cryptic and unnecessary.
IMHO you should have a configurable Snprintf_context.ret field instead.

Code quote:

		int line_no = va_arg(arg, int);
		if (line_no >= 0)
			UT_ASSERTeq(line_no, Snprintf_context.line_no);
		else
			return -1;

---

src/test/core_log_default_function/core_log_default_function.c line 78 at r10 (raw file):

			UT_ASSERTeq(line_no, Snprintf_context.line_no);
		else
			return -1;

Tabs.

Code quote:

		int line_no = va_arg(arg, int);
		if (line_no >= 0)
			UT_ASSERTeq(line_no, Snprintf_context.line_no);
		else
			return -1;

---

src/test/core_log_default_function/core_log_default_function.c line 102 at r10 (raw file):

	UT_OUT("sprintf - %d: %s", RCOUNTER(snprintf), __format);
	UT_FATAL("Unknown sprintf");
	va_end(arg);

Suggestion:

	UT_FATAL("Unexpected #%d sprintf: %s", RCOUNTER(snprintf), __format);

---

src/test/core_log_default_function/core_log_default_function.c line 112 at r10 (raw file):

	char *file_info;
	const char *message;
}Syslog_context;

Suggestion:

} Syslog_context;

---

src/test/core_log_default_function/core_log_default_function.c line 118 at r10 (raw file):

	Syslog_context.no_of_calls++;
	UT_ASSERTeq(__pri, Syslog_context.__pri);
	UT_ASSERTeq(strcmp("%s%s%s", __fmt), 0);

?

Suggestion:

UT_ASSERTstreq(strcmp("%s%s%s", __fmt), 0);

---

src/test/core_log_default_function/core_log_default_function.c line 125 at r10 (raw file):

	char *file_info = va_arg(arg, char *);
	if (Syslog_context.file_info)
		UT_ASSERTstreq(file_info, Syslog_context.file_info);

What file_info should be if the Syslog_context.file_info is not set?

Code quote:

	if (Syslog_context.file_info)
		UT_ASSERTstreq(file_info, Syslog_context.file_info);

---

src/test/core_log_default_function/core_log_default_function.c line 128 at r10 (raw file):

	char *message = va_arg(arg, char *);
	if (Syslog_context.message)
		UT_ASSERTstreq(message, Syslog_context.message);

.

Code quote:

	if (Syslog_context.message)
		UT_ASSERTstreq(message, Syslog_context.message);

---

src/test/core_log_default_function/core_log_default_function.c line 139 at r10 (raw file):

FUNC_MOCK(fprintf, int, FILE *__restrict __stream, \
	const char *__restrict __fmt, ...)

Suggestion:

FUNC_MOCK(fprintf, int, FILE *__restrict __stream, const char *__restrict __fmt,
	...)

---

src/test/core_log_default_function/core_log_default_function.c line 142 at r10 (raw file):

FUNC_MOCK_RUN_DEFAULT {
	UT_ASSERTeq(__stream, stderr);
	Fprintf_context.no_of_calls++;

Please keep the steps in order.

Suggestion:

	Fprintf_context.no_of_calls++;
	UT_ASSERTeq(__stream, stderr);

---

src/test/core_log_default_function/core_log_default_function.c line 147 at r10 (raw file):

		va_start(arg, __fmt);
		char *times_tamp = va_arg(arg, char *);
		UT_ASSERTstreq(times_tamp, Fprintf_context.times_stamp);

What with the rest of the arguments?

---

src/test/core_log_default_function/core_log_default_function.c line 154 at r10 (raw file):

FUNC_MOCK_END

/* Tsts helpers */

Suggestion:

tests' helpers

---

src/test/core_log_default_function/core_log_default_function.c line 162 at r10 (raw file):

static void
TEST_STEP_SETUP(enum core_log_level LEVEL, const char *file_name, int line_no,

Suggestion:

enum core_log_level level

---

src/test/core_log_default_function/core_log_default_function.c line 171 at r10 (raw file):

	Snprintf_context.file_name = file_name;
	Snprintf_context.line_no = line_no;
	Snprintf_context.function_name = function_name;

I think we could drop the effectively constant arguments.

Suggestion:

	Snprintf_context.line_no = LINE_NO;
	Snprintf_context.function_name = FUNCTION_NAME;

---

src/test/core_log_default_function/core_log_default_function.c line 175 at r10 (raw file):

	Fprintf_context.no_of_calls = 0;
	Fprintf_context.times_stamp = NULL;

Redundant new line.

---

src/test/core_log_default_function/core_log_default_function.c line 182 at r10 (raw file):

		UT_ASSERTeq(Syslog_context.no_of_calls, 1); \
		UT_ASSERTeq(Fprintf_context.no_of_calls, FPRINTF_CALLED); \
	} while (0)

Why not a function?

Code quote:

#define TEST_STEP_CHECK(FPRINTF_CALLED) \
	do { \
		UT_ASSERTeq(Syslog_context.no_of_calls, 1); \
		UT_ASSERTeq(Fprintf_context.no_of_calls, FPRINTF_CALLED); \
	} while (0)

---

src/test/core_log_default_function/core_log_default_function.c line 184 at r10 (raw file):

	} while (0)

/* basic tests with a normal message pass through */

Suggestion:

test

---

src/test/core_log_default_function/core_log_default_function.c line 186 at r10 (raw file):

/* basic tests with a normal message pass through */
static int
testDefaultFunction(const struct test_case *tc, int argc, char *argv[])

No camel case! 🛑 🐫 🛑

Please apply to all test names.

Suggestion:

test_default_function

---

src/test/core_log_default_function/core_log_default_function.c line 188 at r10 (raw file):

testDefaultFunction(const struct test_case *tc, int argc, char *argv[])
{
	core_log_set_threshold(CORE_LOG_THRESHOLD_AUX, CORE_LOG_LEVEL_DEBUG);

Redundant.

---

src/test/core_log_default_function/core_log_default_function.c line 194 at r10 (raw file):

		core_log_set_threshold(CORE_LOG_THRESHOLD_AUX, treshold);
		for (enum core_log_level level = CORE_LOG_LEVEL_HARK;
			level < CORE_LOG_LEVEL_MAX; level++) {

Tabs.

Code quote:

		for (enum core_log_level level = CORE_LOG_LEVEL_HARK;
			level < CORE_LOG_LEVEL_MAX; level++) {

---

src/test/core_log_default_function/core_log_default_function.c line 200 at r10 (raw file):

				0, FUNCTION_NAME, MESSAGE);
			TEST_STEP_CHECK(level == CORE_LOG_LEVEL_HARK? 0 :
					level > treshold? 0 : 1);

Please. There is no award for succinctness. There is an award for readability though. ;-)

Suggestion:

			if (level == CORE_LOG_LEVEL_HARK) {
				TEST_STEP_CHECK(0);
			} else {
				TEST_STEP_CHECK(level > treshold ? 0 : 1);
			}

---

src/test/core_log_default_function/core_log_default_function.c line 206 at r10 (raw file):

}

/* test to check that information about bad file is printed */

Suggestion:

test to check that information about a bad file is printed

---

src/test/core_log_default_function/core_log_default_function.c line 216 at r10 (raw file):

		FUNCTION_NAME);
	core_log_default_function(NULL, CORE_LOG_LEVEL_DEBUG, FILE_NAME_W_PATH,
		-1, FUNCTION_NAME, MESSAGE);

Again on the sneaky failure injection. You configure two contradicting line numbers whereas it is expected to succeed for other reasons.
Confusing.

Code quote:

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, FILE_NAME_ERROR, 0,
		FUNCTION_NAME);
	core_log_default_function(NULL, CORE_LOG_LEVEL_DEBUG, FILE_NAME_W_PATH,
		-1, FUNCTION_NAME, MESSAGE);

---

src/test/core_log_default_function/core_log_default_function.c line 231 at r10 (raw file):

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, FILE_NAME, 1, FUNCTION_NAME);
	core_log_default_function(NULL, CORE_LOG_LEVEL_DEBUG, FILE_NAME, 1,
		FUNCTION_NAME, MESSAGE);

Suggestion:

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, FILE_NAME, LINE_NO, FUNCTION_NAME);
	core_log_default_function(NULL, CORE_LOG_LEVEL_DEBUG, FILE_NAME, LINE_NO,
		FUNCTION_NAME, MESSAGE);

---

src/test/core_log_default_function/core_log_default_function.c line 248 at r10 (raw file):

ASSUMPTIONS:

• file == NULL || (file != NULL && function != NULL)

Suggestion:

	core_log_default_function(NULL, CORE_LOG_LEVEL_DEBUG, NULL, 1,
		NULL, MESSAGE);

---

src/test/core_log_default_function/core_log_default_function.c line 264 at r10 (raw file):

	Snprintf_context.forceError = 1;
	Fprintf_context.times_stamp = "[time error] ";
	/* skip file_info snprintf() */

Suggestion:

	Snprintf_context.forceError = 1; /* fail the file_info snprintf() */
	Fprintf_context.times_stamp = "[time error] ";

---

src/test/unittest/unittest.h line 204 at r9 (raw file):

	((void)((__s1 == NULL && __s2 == NULL) || \
		(UT_ASSERT_rt(__s1 != NULL), 1) || \
		(UT_ASSERT_rt(__s2 != NULL), 1) || \

What is the role of these bits?

Suggestion:

		UT_ASSERT_rt(__s1 != NULL) || \
		UT_ASSERT_rt(__s2 != NULL) || \

---

src/test/unittest/unittest.h line 206 at r9 (raw file):

		(UT_ASSERT_rt(__s2 != NULL), 1) || \
		(strcmp(__s1, __s2) == 0) || \
		(UT_ASSERT_MSG("%s: \"%s\" != %s", #__s1, __s1, #__s2), 0)))

Do you intentionally print the second arg name instead of its value?

Suggestion:

(UT_ASSERT_MSG("%s: \"%s\" != %s", #__s1, __s1, __s2), 0)))

---

src/test/unittest/unittest.h line 277 at r9 (raw file):

			ut_fatal(__FILE__, __LINE__, __func__, \
				"UT_ASSERTstreq first param" \
				" must not be const"); \

Suggestion:

			UT_FATAL( \
				"UT_ASSERTstreq: the first param must not be const"); \

[grom72]

Reviewed 3 of 6 files at r11, 1 of 7 files at r12.
Reviewable status: 10 of 18 files reviewed, 37 unresolved discussions (waiting on @janekmi)

---

src/test/core_log_default_function/core_log_default_function.c line 47 at r10 (raw file):

static struct {
	char *ret;
}Strchr_context;

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 52 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not set anywhere explicitly?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 52 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No fail case tested?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 61 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No camel case, please.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 62 at r10 (raw file):

	char *file_info_buffer;
	int forceError;
}Snprintf_context;

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 67 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe all cases should be indented exactly the same as FUNC_MOCK_RUN_DEFAULT.

Can not :(

---

src/test/core_log_default_function/core_log_default_function.c line 71 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Wow. Why it didn't have failed?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 78 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You really don't have to do it. It is really cryptic and unnecessary.
IMHO you should have a configurable Snprintf_context.ret field instead.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 102 at r10 (raw file):

	UT_OUT("sprintf - %d: %s", RCOUNTER(snprintf), __format);
	UT_FATAL("Unknown sprintf");
	va_end(arg);

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 112 at r10 (raw file):

	char *file_info;
	const char *message;
}Syslog_context;

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 118 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 125 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What file_info should be if the Syslog_context.file_info is not set?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 128 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 139 at r10 (raw file):

FUNC_MOCK(fprintf, int, FILE *__restrict __stream, \
	const char *__restrict __fmt, ...)

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 142 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please keep the steps in order.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 154 at r10 (raw file):

FUNC_MOCK_END

/* Tsts helpers */

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 175 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Redundant new line.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 182 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why not a function?

To get assert from proper line.

---

src/test/core_log_default_function/core_log_default_function.c line 184 at r10 (raw file):

	} while (0)

/* basic tests with a normal message pass through */

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 186 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No camel case! 🛑 🐫 🛑

Please apply to all test names.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 188 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Redundant.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 194 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Tabs.

?

---

src/test/core_log_default_function/core_log_default_function.c line 200 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please. There is no award for succinctness. There is an award for readability though. ;-)

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 206 at r10 (raw file):

}

/* test to check that information about bad file is printed */

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 216 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Again on the sneaky failure injection. You configure two contradicting line numbers whereas it is expected to succeed for other reasons.
Confusing.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 231 at r10 (raw file):

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, FILE_NAME, 1, FUNCTION_NAME);
	core_log_default_function(NULL, CORE_LOG_LEVEL_DEBUG, FILE_NAME, 1,
		FUNCTION_NAME, MESSAGE);

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 248 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

ASSUMPTIONS:

• file == NULL || (file != NULL && function != NULL)

No

---

src/test/core_log_default_function/core_log_default_function.c line 264 at r10 (raw file):

	Snprintf_context.forceError = 1;
	Fprintf_context.times_stamp = "[time error] ";
	/* skip file_info snprintf() */

Done.

---

src/test/unittest/unittest.h line 204 at r9 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What is the role of these bits?

Done.

---

src/test/unittest/unittest.h line 206 at r9 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Do you intentionally print the second arg name instead of its value?

Done.

---

src/test/unittest/unittest.h line 277 at r9 (raw file):

			ut_fatal(__FILE__, __LINE__, __func__, \
				"UT_ASSERTstreq first param" \
				" must not be const"); \

Done.

[grom72]

Reviewed 1 of 6 files at r11, 1 of 7 files at r14.
Reviewable status: 11 of 18 files reviewed, 37 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Please update the assumption comment:

Done.

---

---

src/test/core_log_default_function/core_log_default_function.c line 60 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not set?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 78 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Tabs.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 147 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What with the rest of the arguments?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 162 at r10 (raw file):

static void
TEST_STEP_SETUP(enum core_log_level LEVEL, const char *file_name, int line_no,

Done.

[grom72]

Reviewed 5 of 8 files at r13, 2 of 7 files at r14.
Reviewable status: 14 of 18 files reviewed, 37 unresolved discussions (waiting on @janekmi)

---

src/test/core_log_default_function/core_log_default_function.c line 171 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we could drop the effectively constant arguments.

Done.

[janekmi]

Reviewed 1 of 8 files at r13, 6 of 7 files at r14, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @grom72)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

I mean in the src/core/log_default.c file.

---

---

src/test/core_log_default_function/core_log_default_function.c line 157 at r14 (raw file):

FUNC_MOCK_END

/* Testshelpers */

Suggestion:

/* Tests' helpers */

---

src/test/unittest/unittest.h line 277 at r9 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Done.

Nope.

[janekmi]

Reviewed 1 of 7 files at r14, 7 of 7 files at r16, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @grom72)

---

src/test/core_log_default_function/core_log_default_function.c line 48 at r14 (raw file):

const char *Log_level_name;
const char *File_info;

Not used.

---

src/test/core_log_default_function/core_log_default_function.c line 62 at r14 (raw file):

static struct {
	const char *file_name;
	const char *function_name;

FUNCTION_NAME?

---

src/test/core_log_default_function/core_log_default_function.c line 71 at r14 (raw file):

/* file info */
	FUNC_MOCK_RUN(0) {
/* second parameter after __format equal to -1 causes negative value return */

---

src/test/core_log_default_function/core_log_default_function.c line 83 at r14 (raw file):

		/* fill-in the given buffer to verify no memory overwritten */
		memset(__s, 'x', __maxlen);
		*(__s + __maxlen - 1) = '\0';

Considering it is overwritten one line below it seems unnecessary.

---

src/test/core_log_default_function/core_log_default_function.c line 84 at r14 (raw file):

		memset(__s, 'x', __maxlen);
		*(__s + __maxlen - 1) = '\0';
		memcpy(__s, FILE_INFO, sizeof(FILE_INFO) + 1);

sizeof() returns the complete length of the buffer. The terminating null-byte is already accounted for.

Suggestion:

memcpy(__s, FILE_INFO, sizeof(FILE_INFO));

---

src/test/core_log_default_function/core_log_default_function.c line 89 at r14 (raw file):

			return Snprintf_context.ret;
		else
			return __maxlen;

__maxlen means the output has been cropped by one character. Is it intentional?

---

src/test/core_log_default_function/core_log_default_function.c line 96 at r14 (raw file):

		if (Snprintf_context.force_error) {
		/* fill-in the given buffer to verify no memory overwritten */
			memset(__s, 'x', __maxlen);

snprintf() always returns a null-terminated string.

Ref: https://en.cppreference.com/w/c/io/fprintf

---

src/test/core_log_default_function/core_log_default_function.c line 99 at r14 (raw file):

		}
		else
			strncpy(__s, TIMESTAMP, __maxlen);

Suggestion:

		if (Snprintf_context.force_error) {
		/* fill-in the given buffer to verify no memory overwritten */
			memset(__s, 'x', __maxlen);
		} else {
			strncpy(__s, TIMESTAMP, __maxlen);
		}

---

src/test/core_log_default_function/core_log_default_function.c line 100 at r14 (raw file):

		else
			strncpy(__s, TIMESTAMP, __maxlen);
		return __maxlen;

.

---

src/test/core_log_default_function/core_log_default_function.c line 108 at r14 (raw file):

static struct {
	int no_of_calls;

You started using the RCOUNT(func). Why didn't go all the way?

---

src/test/core_log_default_function/core_log_default_function.c line 131 at r14 (raw file):

static struct {
	int no_of_calls;

.

---

src/test/core_log_default_function/core_log_default_function.c line 133 at r14 (raw file):

	int no_of_calls;
	char *times_stamp;
	const char *file_info;

Not used.

---

src/test/core_log_default_function/core_log_default_function.c line 140 at r14 (raw file):

FUNC_MOCK_RUN_DEFAULT {
	Fprintf_context.no_of_calls++;
	UT_ASSERTeq(__stream, stderr);

__fmt is not validated.

---

src/test/core_log_default_function/core_log_default_function.c line 164 at r14 (raw file):

		FUNC_MOCK_RCOUNTER_SET(snprintf, 0); \
		Log_level_name = log_level_names[_LEVEL]; \
		memset(&Syslog_context, 0, sizeof(Syslog_context)); \

Suggestion:

FUNC_MOCK_RCOUNTER_SET(syslog, 0); \

---

src/test/core_log_default_function/core_log_default_function.c line 167 at r14 (raw file):

		Syslog_context.__pri = log_level_syslog_severity[_LEVEL]; \
		Syslog_context.file_info = FILE_INFO; \
		memset(&Snprintf_context, 0, sizeof(Snprintf_context)); \

Would prefer explicit assignments. Especially considering you gain just one line for reduced comprehensibility.

Suggestion:

Snprintf_context.force_error = 0; \
Snprintf_context.ret = 0; \

---

src/test/core_log_default_function/core_log_default_function.c line 170 at r14 (raw file):

		Snprintf_context.file_name = _FILE_NAME_SHORT; \
		Snprintf_context.function_name = _FUNCTION_NAME; \
		memset(&Fprintf_context, 0, sizeof(Fprintf_context)); \

Suggestion:

Fprintf_context.times_stamp = NULL; \

---

src/test/core_log_default_function/core_log_default_function.c line 176 at r14 (raw file):

#define TEST_STEP_CHECK(_FPRINTF_CALLED) \
	do { \

Why RCOUNTER(snprintf) is not validated?

---

src/test/core_log_default_function/core_log_default_function.c line 246 at r14 (raw file):

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, "", FUNCTION_NAME);
	/* skip file_info snprintf() */
	FUNC_MOCK_RCOUNTER_SET(snprintf, 1);

Suggestion:

	FUNC_MOCK_RCOUNTER_SET(snprintf, 1); /* skip file_info snprintf() */

---

src/test/core_log_default_function/core_log_default_function.c line 266 at r16 (raw file):

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, "", FUNCTION_NAME);
	/* skip file_info snprintf() */
	FUNC_MOCK_RCOUNTER_SET(snprintf, 1);

Suggestion:

	
	FUNC_MOCK_RCOUNTER_SET(snprintf, 1); /* skip file_info snprintf() */

[grom72]

Reviewed 4 of 7 files at r14, 2 of 7 files at r15, 1 of 1 files at r19.
Reviewable status: 11 of 18 files reviewed, 20 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I mean in the src/core/log_default.c file.

Done.

---

---

src/test/core_log_default_function/core_log_default_function.c line 67 at r10 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Can not :(

cstyle does not allow.

---

src/test/core_log_default_function/core_log_default_function.c line 48 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not used.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 62 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

FUNCTION_NAME?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 71 at r14 (raw file):

/* file info */
	FUNC_MOCK_RUN(0) {
/* second parameter after __format equal to -1 causes negative value return */

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 83 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Considering it is overwritten one line below it seems unnecessary.

Logic has been changed.

---

src/test/core_log_default_function/core_log_default_function.c line 84 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

sizeof() returns the complete length of the buffer. The terminating null-byte is already accounted for.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 89 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

__maxlen means the output has been cropped by one character. Is it intentional?

Yes

---

src/test/core_log_default_function/core_log_default_function.c line 96 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

snprintf() always returns a null-terminated string.

Ref: https://en.cppreference.com/w/c/io/fprintf

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 99 at r14 (raw file):

		}
		else
			strncpy(__s, TIMESTAMP, __maxlen);

Done

---

src/test/core_log_default_function/core_log_default_function.c line 100 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

?

---

src/test/core_log_default_function/core_log_default_function.c line 108 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You started using the RCOUNT(func). Why didn't go all the way?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 131 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 133 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not used.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 140 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

__fmt is not validated.

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 157 at r14 (raw file):

FUNC_MOCK_END

/* Testshelpers */

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 164 at r14 (raw file):

		FUNC_MOCK_RCOUNTER_SET(snprintf, 0); \
		Log_level_name = log_level_names[_LEVEL]; \
		memset(&Syslog_context, 0, sizeof(Syslog_context)); \

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 167 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Would prefer explicit assignments. Especially considering you gain just one line for reduced comprehensibility.

New logic.

---

src/test/core_log_default_function/core_log_default_function.c line 170 at r14 (raw file):

		Snprintf_context.file_name = _FILE_NAME_SHORT; \
		Snprintf_context.function_name = _FUNCTION_NAME; \
		memset(&Fprintf_context, 0, sizeof(Fprintf_context)); \

New logic.

---

src/test/core_log_default_function/core_log_default_function.c line 176 at r14 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why RCOUNTER(snprintf) is not validated?

Done.

---

src/test/core_log_default_function/core_log_default_function.c line 266 at r16 (raw file):

	TEST_STEP_SETUP(CORE_LOG_LEVEL_DEBUG, "", FUNCTION_NAME);
	/* skip file_info snprintf() */
	FUNC_MOCK_RCOUNTER_SET(snprintf, 1);

Done.

---

src/test/unittest/unittest.h line 277 at r9 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Nope.

cstyle does not allow that

[grom72]

Reviewed 8 of 8 files at r17, 8 of 8 files at r20, 7 of 7 files at r21, 1 of 1 files at r22, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 1 of 8 files at r20, 6 of 7 files at r21, 1 of 1 files at r22, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)

---

src/test/core_log_default_function/core_log_default_function.c line 97 at r22 (raw file):

			return Snprintf.ret;

		memcpy(__s, FILE_INFO, sizeof(FILE_INFO));

You are mixing memcpy() and strncpy() for exactly the same stuff. strncpy()?

---

src/test/core_log_default_function/core_log_default_function.c line 98 at r22 (raw file):

		memcpy(__s, FILE_INFO, sizeof(FILE_INFO));
		return __maxlen;

Nitpick. The return value is not according to the documentation.

Code quote:

		memcpy(__s, FILE_INFO, sizeof(FILE_INFO));
		return __maxlen;

---

src/test/core_log_default_function/core_log_default_function.c line 104 at r22 (raw file):

		UT_ASSERTstreq(__format, "%s.%06ld ");
		strncpy(__s, TIMESTAMP, __maxlen);
		return __maxlen;

Nitpick. The return value is not according to the documentation.

Code quote:

		strncpy(__s, TIMESTAMP, __maxlen);
		return __maxlen;

---

src/test/unittest/unittest.h line 277 at r9 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

cstyle does not allow that

🤦‍♂️

[grom72]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)

---

src/test/core_log_default_function/core_log_default_function.c line 97 at r22 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You are mixing memcpy() and strncpy() for exactly the same stuff. strncpy()?

Done.

[janekmi]

🐐

Reviewed 1 of 1 files at r23, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

---

src/test/core_log_default_function/core_log_default_function.c line 97 at r23 (raw file):

			return Snprintf.ret;
		UT_ASSERT(sizeof(FILE_INFO) <= __maxlen);
		strcpy(__s, FILE_INFO);

You might get a Coverity issue by using non-n variant.

Suggestion:

strncpy(__s, FILE_INFO, __maxlen);

---

src/test/core_log_default_function/core_log_default_function.c line 104 at r23 (raw file):

		UT_ASSERTstreq(__format, "%s.%06ld ");
		UT_ASSERT(sizeof(TIMESTAMP) <= __maxlen);
		strcpy(__s, TIMESTAMP);

.

[osalyk]

Reviewed 10 of 16 files at r9, 1 of 8 files at r20, 6 of 7 files at r21, 1 of 1 files at r23, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

[grom72]

Reviewed 5 of 7 files at r24, 7 of 7 files at r25, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

[grom72]

Reviewed 6 of 7 files at r26, 7 of 7 files at r27, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)

[janekmi]

Reviewed 7 of 7 files at r27, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)