test: pmemobj_log_function UT implementation

[grom72]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.11%. Comparing base (b8f3dc2) to head (0ad2b2b).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6048      +/-   ##
==========================================
+ Coverage   70.07%   70.11%   +0.03%     
==========================================
  Files         133      133              
  Lines       19563    19560       -3     
  Branches     3261     3261              
==========================================
+ Hits        13709    13714       +5     
+ Misses       5854     5846       -8     

[osalyk]

Reviewed 1 of 6 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

[janekmi]

Reviewed 4 of 5 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @grom72)

a discussion (no related file):
Since you are testing all the log APIs I believe this test should be called obj_log.

---

a discussion (no related file):
You basically glued together three unit tests. It was even more natural for you to keep separate mocks for each of the get/set threshold and set_function tests. Please reconsider if you really want to keep them this way. TBH at first I missed the first test because of how this file is organized.

---

---

src/include/libpmemobj/log.h line 34 at r4 (raw file):

	PMEMOBJ_LOG_LEVEL_INFO,
	/* debug info e.g. write operation dump */
	PMEMOBJ_LOG_LEVEL_DEBUG,

FYI I don't think removing commas at the end of an enum is valuable in of itself. A comma here is syntactically correct. The end of the list is indicated by the curly bracket below.

I consider it handy since adding new values will not require modifying neighbouring lines.

The bottom line is it is a matter of taste and personal preference.

---

src/test/obj_log_function/Makefile line 12 at r4 (raw file):

# 'internal' is required for proper mock integration
# 'debug' is required for debug version of core/log.o that provides
# implementation of 'out_log()' that is used by 'ut_log_function()'

This wording is no longer true. You can make use of mine as proposed in other Makefile files.

Suggestion:

# required for proper mock integration

---

src/test/obj_log_function/obj_log_function.c line 11 at r4 (raw file):

#include <string.h>
#include <syslog.h>
#include <stdbool.h>

I believe none of these is necessary.

---

src/test/obj_log_function/obj_log_function.c line 36 at r4 (raw file):

	FUNC_MOCK_RUN(VALIDATED_CALL) {
		UT_ASSERT((void *)log_function ==
			(void *)PMEMOBJ_LOG_CUSTOM_FUNCTION_MOCK);

Suggestion:

		UT_ASSERTeq((void *)log_function,
			(void *)PMEMOBJ_LOG_CUSTOM_FUNCTION_MOCK);

---

src/test/obj_log_function/obj_log_function.c line 49 at r4 (raw file):

{
	errno = NO_ERRNO;
	Core_log_set_function.ret = error == NO_ERRNO? 0: error;

Atypical spacing.

Suggestion:

Core_log_set_function.ret = error == NO_ERRNO ? 0 : error;

---

src/test/obj_log_function/obj_log_function.c line 57 at r4 (raw file):

		UT_ASSERTeq(ret, 1);
	}
	UT_ASSERTeq(errno, error);

The errno value on success is undefined.

Suggestion:

	if (error == NO_ERRNO) {
		UT_ASSERTeq(ret, 0);
	} else {
		UT_ASSERTeq(ret, 1);
		UT_ASSERTeq(errno, error);
	}

---

src/test/obj_log_function/obj_log_function.c line 92 at r4 (raw file):

 * *** pmemobj_log_set_treshold() tests ***
 */
static enum core_log_threshold tresholds[] = {

Suggestion:

core_tresholds

---

src/test/obj_log_function/obj_log_function.c line 95 at r4 (raw file):

	CORE_LOG_THRESHOLD, CORE_LOG_THRESHOLD_AUX};

static enum core_log_level levels[] = {

Suggestion:

core_levels

---

src/test/obj_log_function/obj_log_function.c line 98 at r4 (raw file):

	CORE_LOG_LEVEL_HARK, CORE_LOG_LEVEL_FATAL, CORE_LOG_LEVEL_ERROR,
	CORE_LOG_LEVEL_WARNING, CORE_LOG_LEVEL_NOTICE, CORE_LOG_LEVEL_INFO,
	CORE_LOG_LEVEL_DEBUG};

IMHO it is easier to follow when kept the same way as in the place where they are defined.

Suggestion:

	CORE_LOG_LEVEL_HARK,
	CORE_LOG_LEVEL_FATAL,
	CORE_LOG_LEVEL_ERROR,
	CORE_LOG_LEVEL_WARNING,
	CORE_LOG_LEVEL_NOTICE,
	CORE_LOG_LEVEL_INFO,
	CORE_LOG_LEVEL_DEBUG
};

---

src/test/obj_log_function/obj_log_function.c line 124 at r4 (raw file):

{
	errno = NO_ERRNO;
	Core_log_set_treshold.ret = error == NO_ERRNO? 0: error;

Atypical spacing.

Suggestion:

Core_log_set_treshold.ret = error == NO_ERRNO ? 0 : error;

---

src/test/obj_log_function/obj_log_function.c line 139 at r4 (raw file):

				UT_ASSERTeq(ret, 1);
			}
			UT_ASSERTeq(errno, error);

.

Suggestion:

			if (error == NO_ERRNO) {
				UT_ASSERTeq(ret, 0);
			} else {
				UT_ASSERTeq(ret, 1);
				UT_ASSERTeq(errno, error);
			}

---

src/test/obj_log_function/obj_log_function.c line 205 at r4 (raw file):

{
	errno = NO_ERRNO;
	Core_log_get_treshold.ret = error == NO_ERRNO? 0: error;

.

Suggestion:

Core_log_get_treshold.ret = error == NO_ERRNO ? 0 : error;

---

src/test/obj_log_function/obj_log_function.c line 222 at r4 (raw file):

				UT_ASSERTeq(ret, 1);
			}
			UT_ASSERTeq(errno, error);

Suggestion:

			if (error == NO_ERRNO) {
				UT_ASSERTeq(ret, 0);
				UT_ASSERTeq(level, exp_level);
			} else {
				UT_ASSERTeq(ret, 1);
				UT_ASSERTeq(errno, error);
			}

---

src/test/obj_log_function/TESTS.py line 15 at r4 (raw file):

# once. No dynamic libraries are used nor .static_* builds are available.
@t.require_build('debug')
class CORE_LOG(t.BaseTest):

We are leaving the core realm for good. ;-)

Suggestion:

OBJ_LOG

[grom72]

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @janekmi)

---

src/test/obj_log_function/obj_log_function.c line 57 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

The errno value on success is undefined.

No, we should also check that the errno is left unchanged after a valid operation.

---

src/test/obj_log_function/obj_log_function.c line 139 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

---

src/test/obj_log_function/obj_log_function.c line 222 at r4 (raw file):

				UT_ASSERTeq(ret, 1);
			}
			UT_ASSERTeq(errno, error);

.

[grom72]

Reviewable status: 0 of 15 files reviewed, 13 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/test/obj_log_function/obj_log_function.c line 11 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe none of these is necessary.

Done.

---

src/test/obj_log_function/obj_log_function.c line 36 at r4 (raw file):

	FUNC_MOCK_RUN(VALIDATED_CALL) {
		UT_ASSERT((void *)log_function ==
			(void *)PMEMOBJ_LOG_CUSTOM_FUNCTION_MOCK);

Done.

---

src/test/obj_log_function/obj_log_function.c line 57 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

No, we should also check that the errno is left unchanged after a valid operation.

Done

---

src/test/obj_log_function/obj_log_function.c line 92 at r4 (raw file):

 * *** pmemobj_log_set_treshold() tests ***
 */
static enum core_log_threshold tresholds[] = {

Done

---

src/test/obj_log_function/obj_log_function.c line 95 at r4 (raw file):

	CORE_LOG_THRESHOLD, CORE_LOG_THRESHOLD_AUX};

static enum core_log_level levels[] = {

Done

---

src/test/obj_log_function/obj_log_function.c line 98 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO it is easier to follow when kept the same way as in the place where they are defined.

Done

---

src/test/obj_log_function/obj_log_function.c line 139 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done

---

src/test/obj_log_function/obj_log_function.c line 205 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done

---

src/test/obj_log_function/obj_log_function.c line 222 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done

---

src/test/obj_log_function/TESTS.py line 15 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We are leaving the core realm for good. ;-)

Done.

[grom72]

Reviewable status: 0 of 15 files reviewed, 12 unresolved discussions (waiting on @janekmi and @osalyk)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Since you are testing all the log APIs I believe this test should be called obj_log.

Done.

---

[grom72]

Reviewable status: 0 of 15 files reviewed, 11 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/test/obj_log_function/Makefile line 12 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This wording is no longer true. You can make use of mine as proposed in other Makefile files.

Done.

[osalyk]

Reviewed 1 of 6 files at r6, 3 of 6 files at r8, 11 of 11 files at r9, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/test/obj_log_set_treshold/obj_log_set_treshold.c line 5 at r9 (raw file):

/*
 * obj_log_set_treshold.c -- unit test for pmemobj_log_set_function

Suggestion:

obj_log_set_treshold.c -- unit test for pmemobj_log_set_treshold

---

src/test/obj_log_set_function/obj_log_set_function.c line 5 at r9 (raw file):

/*
 * obj_log_set_function.c -- unit test for pmemobj_log_set_treshold

Suggestion:

obj_log_set_function.c -- unit test for pmemobj_log_set_function

[grom72]

Reviewable status: 4 of 15 files reviewed, 13 unresolved discussions (waiting on @janekmi and @osalyk)

---

src/test/obj_log_set_treshold/obj_log_set_treshold.c line 5 at r9 (raw file):

/*
 * obj_log_set_treshold.c -- unit test for pmemobj_log_set_function

Done.

---

src/test/obj_log_set_function/obj_log_set_function.c line 5 at r9 (raw file):

/*
 * obj_log_set_function.c -- unit test for pmemobj_log_set_treshold

Done.

[osalyk]

Reviewed 11 of 11 files at r11, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 1 of 6 files at r6, 3 of 6 files at r8, 11 of 11 files at r11, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @grom72)

---

src/test/obj_log_get_treshold/obj_log_get_treshold.c line 57 at r11 (raw file):

{
	errno = NO_ERRNO;
	Core_log_get_treshold.ret = error == NO_ERRNO ? 0: error;

Suggestion:

Core_log_get_treshold.ret = error == NO_ERRNO ? 0 : error;

---

src/test/obj_log_get_treshold/obj_log_get_treshold.c line 71 at r11 (raw file):

				UT_ASSERTeq(ret, 0);
				UT_ASSERTeq(level, exp_level);
				UT_ASSERTeq(errno, NO_ERRNO);

No guarantees on the errno value when ret == 0.

---

src/test/obj_log_set_treshold/obj_log_set_treshold.c line 56 at r11 (raw file):

{
	errno = NO_ERRNO;
	Core_log_set_treshold.ret = error == NO_ERRNO ? 0: error;

Suggestion:

Core_log_set_treshold.ret = error == NO_ERRNO ? 0 : error;

---

src/test/obj_log_set_treshold/obj_log_set_treshold.c line 68 at r11 (raw file):

			if (error == NO_ERRNO) {
				UT_ASSERTeq(ret, 0);
				UT_ASSERTeq(errno, NO_ERRNO);

.

---

src/test/obj_log_set_treshold/TESTS.py line 21 at r11 (raw file):

        ctx.exec('obj_log_set_treshold', self.test_case)

You are missing the test_log_set_treshold case.

---

src/test/obj_log_set_function/obj_log_set_function.c line 40 at r11 (raw file):

{
	errno = NO_ERRNO;
	Core_log_set_function.ret = error == NO_ERRNO ? 0: error;

Suggestion:

Core_log_set_function.ret = error == NO_ERRNO ? 0 : error;

---

src/test/obj_log_set_function/obj_log_set_function.c line 45 at r11 (raw file):

	if (error == NO_ERRNO) {
		UT_ASSERTeq(ret, 0);
		UT_ASSERTeq(errno, NO_ERRNO);

[grom72]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @janekmi)

---

src/test/obj_log_get_treshold/obj_log_get_treshold.c line 71 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No guarantees on the errno value when ret == 0.

We know that errno is used to report errors. We do not have UT for core_log_error_translate.
So either we test it like it is or we have to add UT for core_log_error_translate.

---

src/test/obj_log_set_treshold/TESTS.py line 21 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You are missing the test_log_set_treshold case.

Done.
:)

[janekmi]

Reviewed 4 of 4 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72)

---

src/test/obj_log_get_treshold/obj_log_get_treshold.c line 71 at r11 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

We know that errno is used to report errors. We do not have UT for core_log_error_translate.
So either we test it like it is or we have to add UT for core_log_error_translate.

Again. No specification I read in my life specifies what errno value you should expect when ret == 0. So, this check asserts a thing we have not ever committed to assure.

[janekmi]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72)

a discussion (no related file):
Please resolve conflicts.

---

[grom72]

Reviewable status: 12 of 15 files reviewed, 4 unresolved discussions (waiting on @janekmi)

---

src/test/obj_log_get_treshold/obj_log_get_treshold.c line 71 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Again. No specification I read in my life specifies what errno value you should expect when ret == 0. So, this check asserts a thing we have not ever committed to assure.

Done.

---

src/test/obj_log_set_treshold/obj_log_set_treshold.c line 68 at r11 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/test/obj_log_set_function/obj_log_set_function.c line 45 at r11 (raw file):

	if (error == NO_ERRNO) {
		UT_ASSERTeq(ret, 0);
		UT_ASSERTeq(errno, NO_ERRNO);

Done.

[grom72]

Reviewable status: 12 of 15 files reviewed, 4 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Please resolve conflicts.

Done.

---

[janekmi]

Reviewed 4 of 14 files at r14, 3 of 9 files at r16, 11 of 13 files at r17, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @grom72)