-
Notifications
You must be signed in to change notification settings - Fork 510
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
common: core_log_set/get_threshold MT unit tests (part 2) #6038
Conversation
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.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)
a discussion (no related file):
Shall we mock ut_log_function
or log_default_function
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6038 +/- ##
==========================================
+ Coverage 70.08% 70.09% +0.01%
==========================================
Files 133 133
Lines 19563 19560 -3
Branches 3262 3261 -1
==========================================
Hits 13711 13711
+ Misses 5852 5849 -3 |
6259584
to
2118a7e
Compare
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.
Reviewed 8 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk)
a discussion (no related file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Shall we mock
ut_log_function
orlog_default_function
I would argue it is unnecessary:
- It would significantly complicate the implementation.
- Made no difference whether the logging message is or is not further processed. The key thing is to hit the condition below.
- I just assured no other function will touch the
Core_log_threshold[]
directly.
2118a7e
to
6d1e490
Compare
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.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk)
6d1e490
to
8c2e818
Compare
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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk)
a discussion (no related file):
Previously, janekmi (Jan Michalski) wrote…
I would argue it is unnecessary:
- It would significantly complicate the implementation.
- Made no difference whether the logging message is or is not further processed. The key thing is to hit the condition below.
- I just assured no other function will touch the
Core_log_threshold[]
directly.
Considering what we discussed offline, it is no longer relevant.
8c2e818
to
802075c
Compare
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.
Reviewed 7 of 9 files at r1, 1 of 3 files at r2, 2 of 3 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
802075c
to
d95effd
Compare
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.
Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
d95effd
to
ff44749
Compare
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.
Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
ff44749
to
44cbd4a
Compare
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.
Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
44cbd4a
to
93efca6
Compare
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.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
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.
Reviewed 2 of 2 files at r6, 2 of 3 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
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.
Reviewed 1 of 3 files at r2, 1 of 2 files at r6, 1 of 3 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)
src/core/log_internal.h
line 93 at r9 (raw file):
_Atomic #endif /* ATOMIC_OPERATIONS_SUPPORTED */ uintptr_t Core_log_function;
What has happened here?
Code quote:
/* pointer to the logging function */
extern
#ifdef ATOMIC_OPERATIONS_SUPPORTED
_Atomic
#endif /* ATOMIC_OPERATIONS_SUPPORTED */
uintptr_t Core_log_function;
src/test/core_log_threshold_mt/core_log_threshold_mt.c
line 49 at r9 (raw file):
CORE_LOG_LEVEL_MAX); int ret = core_log_set_threshold(ctx->threshold, level); UT_ASSERT(ret == 0 || ret == EAGAIN);
It will be useful to print info when EAGAIN is returned by the function.
Suggestion:
UT_ASSERT(ret == 0 || ret == EAGAIN);
if (ret == EAGAIN)
UT_OUT( "ret == EAGAIN");
93efca6
to
0917d29
Compare
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.
Reviewed 1 of 3 files at r10.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)
src/core/log_internal.h
line 93 at r9 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
What has happened here?
Done.
0917d29
to
e06c162
Compare
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.
Reviewed 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72)
src/test/core_log_threshold_mt/core_log_threshold_mt.c
line 49 at r9 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
It will be useful to print info when EAGAIN is returned by the function.
Done.
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.
Reviewed 3 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)
- make core_log_threshold_mt a dedicated test - keep Core_log_threshold available only via API - add _core_log_get_threshold_internal() calls Ref: pmem#6035 Signed-off-by: Jan Michalski <jan.michalski@intel.com>
Ref: #6035
This change is