Skip to content
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

test: don't configure the log function for PMEMOBJ nor PMEMCORE #6044

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Mar 8, 2024

@janekmi janekmi added sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Mar 8, 2024
@janekmi janekmi requested review from grom72 and osalyk March 8, 2024 15:23
Copy link
Contributor Author

@janekmi janekmi left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72 and @osalyk)


src/test/Makefile.inc line 388 at r1 (raw file):

ifeq ($(LIBPMEMCORE), y)
LIBPMEM=y
USE_LOG_PMEMCORE=y

USE_LOG_PMEMCORE is not necessary for unittest.h anymore. And since it is a misnomer without it and logically it is just USE_LOG_PMEMCORE == '' I decided to just drop it.


src/test/Makefile.inc line 536 at r1 (raw file):

ifeq ($(USE_LOG_PMEMCORE),y)
CFLAGS += -DUSE_LOG_PMEMCORE
endif

The USE_LOG_PMEMCORE define is no longer needed for unittest.h.

Code quote:

ifeq ($(USE_LOG_PMEMCORE),y)
CFLAGS += -DUSE_LOG_PMEMCORE
endif

src/test/Makefile.inc line 536 at r1 (raw file):

# LIBPMEMCORE == '' means the binary is built without directly linking with
# core units which are required whenever test makes use of core utils.

As far as I can tell, these objects are necessary only because tests also use utils from the core directory which may from time to time come with either a function body defined in one of them or call one of the logging functions.

Code quote:

# LIBPMEMCORE == '' means the binary is built without directly linking with
# core units which are required whenever test makes use of core utils.

src/test/core_log/Makefile.inc line 8 at r1 (raw file):

# 'internal' is required for proper mock integration
# 'debug' provides 'out_log()' required for 'ut_log_function()'

internal is still needed for mocking. Whereas debug is necessary since the nondebug variant is more integrated and functions we want to mock may no longer be named as we know them. xD

Code quote:

# 'internal' is required for proper mock integration
# 'debug' provides 'out_log()' required for 'ut_log_function()'

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.07%. Comparing base (b00e032) to head (6f2c54a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6044      +/-   ##
==========================================
- Coverage   70.09%   70.07%   -0.03%     
==========================================
  Files         133      133              
  Lines       19560    19563       +3     
  Branches     3261     3261              
==========================================
- Hits        13710    13708       -2     
- Misses       5850     5855       +5     

@janekmi janekmi force-pushed the test-no-pmemobj_log_set_function branch from 3633bc9 to 62299c5 Compare March 9, 2024 12:31
Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 9 files at r1, 14 of 16 files at r2, 1 of 1 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)

a discussion (no related file):
Please wrap out_log() by ifdef DEBUG ... in out.h as it is no longer needed in a nondebug version.



src/test/core_log/Makefile.inc line 8 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

internal is still needed for mocking. Whereas debug is necessary since the nondebug variant is more integrated and functions we want to mock may no longer be named as we know them. xD

So why do you want to remove this comment?


src/test/core_log_default_function/Makefile line 10 at r4 (raw file):

BUILD_STATIC_NONDEBUG=n

# required for proper mock integration

.

Code quote:

# required for proper mock integration

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 9 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 9 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi and @osalyk)


src/test/core_log/Makefile.inc line 7 at r1 (raw file):

BUILD_STATIC_NONDEBUG=n

# required for proper mock integration

.

Code quote:

# required for proper mock integration

@janekmi janekmi force-pushed the test-no-pmemobj_log_set_function branch from 41727d9 to ea6955c Compare March 11, 2024 11:13
Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 14 of 22 files reviewed, 3 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Please wrap out_log() by ifdef DEBUG ... in out.h as it is no longer needed in a nondebug version.

Done.


Copy link
Contributor

@grom72 grom72 left a 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 r5.
Reviewable status: 15 of 22 files reviewed, 3 unresolved discussions (waiting on @osalyk)

@janekmi janekmi force-pushed the test-no-pmemobj_log_set_function branch from ea6955c to 6f2c54a Compare March 11, 2024 16:53
Copy link
Contributor Author

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 11 files at r8, all commit messages.
Reviewable status: 12 of 22 files reviewed, 3 unresolved discussions (waiting on @grom72 and @osalyk)


src/test/core_log/Makefile.inc line 7 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

.


src/test/core_log/Makefile.inc line 8 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

So why do you want to remove this comment?

It is not a removal. It is rephrasing. The internal-debug is still necessary but for slightly different reasons which I expressed as presented in the PR. If you would like to have it put down differently I am open to discussing your proposition.


src/test/core_log_default_function/Makefile line 10 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

.

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 11 files at r7, 11 of 11 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

... nor PMEMCORE.

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
Signed-off-by: Jan Michalski <jan.michalski@intel.com>
Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 16 files at r2, 10 of 10 files at r4, 1 of 1 files at r5, 11 of 11 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit d16a3f1 into master Mar 12, 2024
8 checks passed
@janekmi janekmi deleted the test-no-pmemobj_log_set_function branch April 16, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Add to skip the changelog check on your pull request sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants