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: remove libpmemlog tests and examples #5752

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

osalyk
Copy link
Contributor

@osalyk osalyk commented Jun 27, 2023

This change is Reviewable

@osalyk osalyk added the sprint goal This pull request is part of the ongoing sprint label Jun 27, 2023
@osalyk osalyk added this to the 2.0.0 milestone Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #5752 (6ada149) into master (34f14bb) will decrease coverage by 1.11%.
The diff coverage is n/a.

❗ Current head 6ada149 differs from pull request most recent head af002a6. Consider uploading reports for the commit af002a6 to get more accurate results

@@            Coverage Diff             @@
##           master    #5752      +/-   ##
==========================================
- Coverage   70.95%   69.85%   -1.11%     
==========================================
  Files         143      143              
  Lines       21985    22493     +508     
  Branches     3698     3763      +65     
==========================================
+ Hits        15600    15713     +113     
- Misses       6385     6780     +395     

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.

Please take a look at:
https://github.com/osalyk/pmdk/blob/177e2b2ace4dd623a839fc52e8eb5e3fc7b13859/src/test/compat_incompat_features/common.sh#L21
POOL_TYPES=(obj blk log) - > POOL_TYPES=(obj log)
https://github.com/osalyk/pmdk/blob/177e2b2ace4dd623a839fc52e8eb5e3fc7b13859/src/test/compat_incompat_features/common.sh#L26
create_args[blk]="blk 512 $POOLSET"
and also at:
https://github.com/osalyk/pmdk/blob/177e2b2ace4dd623a839fc52e8eb5e3fc7b13859/src/test/libpmempool_backup/common.sh#L21

POOL_TYPES=( log obj )
POOL_CREATE_PARAMS=( "" "--layout test_layout" )
POOL_CHECK_PARAMS=( "-s" "-soOaAbZH -l -C" )

Reviewable status: 0 of 109 files reviewed, all discussions resolved

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.

Sorry, my mistake the above lines remove libpmemblk tests but I thought about libpmemlog.

Reviewable status: 0 of 109 files reviewed, all discussions resolved

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.

Please add these changes to Changelog

Reviewed 3 of 109 files at r1, all commit messages.
Reviewable status: 3 of 109 files reviewed, all discussions resolved

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 76 of 109 files at r1.
Reviewable status: 79 of 109 files reviewed, all discussions resolved

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 30 of 109 files at r1.
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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

a discussion (no related file):
Please add these changes to Changelog


@osalyk osalyk changed the title test: remove libpmemlog tests test: remove libpmemlog tests and examples Jun 27, 2023
@osalyk
Copy link
Contributor Author

osalyk commented Jun 27, 2023

Please take a look at: https://github.com/osalyk/pmdk/blob/177e2b2ace4dd623a839fc52e8eb5e3fc7b13859/src/test/compat_incompat_features/common.sh#L21 POOL_TYPES=(obj blk log) - > POOL_TYPES=(obj log) https://github.com/osalyk/pmdk/blob/177e2b2ace4dd623a839fc52e8eb5e3fc7b13859/src/test/compat_incompat_features/common.sh#L26 create_args[blk]="blk 512 $POOLSET" and also at: https://github.com/osalyk/pmdk/blob/177e2b2ace4dd623a839fc52e8eb5e3fc7b13859/src/test/libpmempool_backup/common.sh#L21

POOL_TYPES=( log obj )
POOL_CREATE_PARAMS=( "" "--layout test_layout" )
POOL_CHECK_PARAMS=( "-s" "-soOaAbZH -l -C" )

Reviewable status: 0 of 109 files reviewed, all discussions resolved

I will make changes to the (lib)pmempool_* tests in a separate PR.

Copy link
Contributor Author

@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.

Reviewable status: 109 of 147 files reviewed, 1 unresolved discussion (waiting on @grom72)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Please add these changes to Changelog

Done.


Copy link
Contributor

@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.

Just make sure to keep your local master up to date and take a look at other PRs before starting any changes in these directories. I see a potential for overlap here.

Reviewable status: 109 of 147 files reviewed, 1 unresolved discussion (waiting on @grom72)

Copy link
Contributor

@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 14 of 109 files at r1, 35 of 38 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72 and @osalyk)


ChangeLog line 3 at r3 (raw file):

	* Unreleased *

	This release :

What kind of formatting is it? xD


ChangeLog line 4 at r3 (raw file):


	This release :
	- remove libpmemlog tests and examples

removes


src/test/ctl_cow/TEST1 line 0 at r3 (raw file):
We have a slight overlap here: https://github.com/pmem/pmdk/pull/5750/files#diff-0229b322d0c495f758542c99b83a8434d0c03a15c24380d0e61cc2fbe8b24101

I am happy to merge yours first, possibly one from @grom72 removing the blk tests as well and closing mine altogether.
You are approach is a lot more robust in this regard.


src/test/ctl_prefault/TEST2 line 0 at r3 (raw file):
The same here: https://github.com/pmem/pmdk/pull/5750/files#diff-5208c8587590f4ab83cda8ac1cef16976dadf4c70398a5376644f6b9315181f1


src/test/tools/pmemwrite/write.c line 215 at r3 (raw file):

		break;
	case PMEM_POOL_TYPE_LOG:
		ret = pmemwrite_log(&pmemwrite);

Without (lib)pmempool tests being adjusted to stop using log and blk pools you might experience fails.
I hope my changes sort it out: #5745 #5749 #5748 #5746

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 35 of 38 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (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: all files reviewed, 2 unresolved discussions (waiting on @osalyk)

Copy link
Contributor Author

@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.

Reviewable status: 144 of 147 files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)


ChangeLog line 3 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What kind of formatting is it? xD

Done.


ChangeLog line 4 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

removes

Done.


src/test/tools/pmemwrite/write.c line 215 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Without (lib)pmempool tests being adjusted to stop using log and blk pools you might experience fails.
I hope my changes sort it out: #5745 #5749 #5748 #5746

I withdrew from these changes. I will add them in the next PR

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 3 files at r4.
Reviewable status: 145 of 147 files reviewed, 3 unresolved discussions (waiting on @janekmi and @osalyk)


src/test/tools/pmemwrite/write.c line 252 at r4 (raw file):

		break;
	case PMEM_POOL_TYPE_OBJ:
		ret = pmemwrite_obj(&pmemwrite);

It is also a part of my changes, but I think the first PR is the winner.

Code quote:

		break;
	case PMEM_POOL_TYPE_LOG:
		ret = pmemwrite_log(&pmemwrite);
		break;
	case PMEM_POOL_TYPE_OBJ:
		ret = pmemwrite_obj(&pmemwrite);

Copy link
Contributor

@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 95 of 109 files at r1, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)


src/test/tools/pmemwrite/Makefile line 25 at r4 (raw file):

# Libpmemblk and libpmemlog are deprecated.
# This flag allows to build tests, examples and benchmarks
# using pmemblk/pmemlog despite the deprecated state.

I guess this comment should be reverted to the original state as well.

Copy link
Contributor Author

@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.

Reviewable status: 143 of 147 files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)


src/test/tools/pmemwrite/Makefile line 25 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I guess this comment should be reverted to the original state as well.

Done.


src/test/tools/pmemwrite/write.c line 252 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

It is also a part of my changes, but I think the first PR is the winner.

I backed off these changes until pmempool was cleaned up

Copy link
Contributor

@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.

:lgtm:

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)

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 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)


src/test/tools/pmemwrite/Makefile line 15 at r5 (raw file):

LIBPMEM=y
LIBPMEMBLK=y
LIBPMEMLOG=y

Do we need this?

Code quote:

LIBPMEMLOG=y

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: all files reviewed, 1 unresolved discussion (waiting on @osalyk)


src/test/tools/pmemwrite/write.c line 252 at r4 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

I backed off these changes until pmempool was cleaned up

Yes, first we have to cleanup tests

Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)


src/test/tools/pmemwrite/Makefile line 15 at r5 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Do we need this?

The file has been restored

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 3 files at r4.
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)

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 4 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link
Contributor

@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 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

@janekmi janekmi merged commit 8220f51 into pmem:master Jun 28, 2023
24 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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