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: replace deprecated libpmempool_api tests #5745

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Jun 23, 2023

  • drop tests if log, blk and btt specific
  • replace log, blk and btt using tests with obj tests
  • pmemwrite has been extended to support PMEMOBJ pools as well

This change is Reviewable

@janekmi janekmi added this to the 2.0.0 milestone Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #5745 (627e970) into master (54c43c6) will decrease coverage by 0.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5745      +/-   ##
==========================================
- Coverage   73.89%   73.70%   -0.20%     
==========================================
  Files         143      143              
  Lines       21978    21978              
  Branches     3696     3697       +1     
==========================================
- Hits        16240    16198      -42     
- Misses       5738     5780      +42     

@janekmi janekmi added the sprint goal This pull request is part of the ongoing sprint label Jun 26, 2023
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 31 of 31 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)


src/test/libpmempool_api/TEST0 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 1 -a 0 -r 1 -t obj $POOL >> $LOG

Would it be better to change default -t value from log -> obj?

Code quote:

-t obj 

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 9 of 31 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi)

a discussion (no related file):
Do we plan to make another PR to remove all -t obj as we will support only one type of pool?
Or perhaps we need at first a PR that makes -t mandatory also for log tests?
libpmempool_test is used without -t only for libpmempool_api where we want to support only obj related tests. In the rest of tests -t is given explicitly - no risk of any side effects.



src/test/libpmempool_api/libpmempool_test.c line 105 at r1 (raw file):

		.path		= NULL,
		.backup_path	= NULL,
		.pool_type	= PMEMPOOL_POOL_TYPE_DETECT,

Can we assume that we support only one type of pool, the obj one?
In such case, we can skip -t obj in all tests.

Suggestion:

PMEMPOOL_POOL_TYPE_OBJ

src/test/libpmempool_api/out0.log.match line 8 at r1 (raw file):

pool header correct
checking pmemlog header
pmemlog header correct

Should we not have here some obj-related messages?


src/test/libpmempool_api/out1.log.match line 8 at r1 (raw file):

pool header correct
checking pmemlog header
pmemlog header correct

.


src/test/libpmempool_api/out10.log.match line 16 at r1 (raw file):

incorrect pool header
the repair of pmemobj pools is not supported
status = cannot repair

What is the value of this test if it can not complete w/ obj correctly?


src/test/libpmempool_api/out13.log.match line 12 at r1 (raw file):

pool header correct
checking pmemlog header
pmemlog header correct

.

Code quote:

checking pmemlog header
pmemlog header correct

src/test/libpmempool_api/out9.log.match line 8 at r1 (raw file):

incorrect pool header
the repair of pmemobj pools is not supported
status = cannot repair

What is the purpose of this test?

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 22 of 31 files at r1.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @janekmi)


src/test/libpmempool_api/out4.log.match line 2 at r1 (raw file):

libpmempool_api$(nW)TEST4: START: libpmempool_test$(nW)
 $(nW)libpmempool_test$(nW) -d 1 -a 1 -r 0 -t obj $(nW)

.


src/test/libpmempool_api/out5.log.match line 2 at r1 (raw file):

libpmempool_api$(nW)TEST5: START: libpmempool_test$(nW)
 $(nW)libpmempool_test$(nW) -d 1 -b $(nW).backup -t obj $(nW)

.


src/test/libpmempool_api/out6.log.match line 2 at r1 (raw file):

libpmempool_api$(nW)TEST6: START: libpmempool_test$(nW)
 $(nW)libpmempool_test$(nW) -s 0 -t obj $(nW)

.


src/test/libpmempool_api/out7.log.match line 2 at r1 (raw file):

libpmempool_api$(nW)TEST7: START: libpmempool_test$(nW)
 $(nW)libpmempool_test$(nW) -r 0 -y 1 -t obj $(nW)

.


src/test/libpmempool_api/out8.log.match line 3 at r1 (raw file):

libpmempool_api$(nW)TEST8: START: libpmempool_test$(nW)
 $(nW)libpmempool_test$(nW) -t blk $(nW)
Error: Invalid argument

Shall we keep this test with some random - non 'obj' value?

Code quote:

Invalid argument

src/test/libpmempool_api/TEST1 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 0 -a 1 -r 1 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST10 line 28 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX \
	-d 0 -r 0 -y 0 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST10 line 37 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 0 -r 1 -y 1 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST13 line 25 at r1 (raw file):

expect_normal_exit $PMEMPOOL$EXESUFFIX create obj $POOL

expect_normal_exit ./libpmempool_test$EXESUFFIX -s 0 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST13 line 27 at r1 (raw file):

expect_normal_exit ./libpmempool_test$EXESUFFIX -s 0 -t obj $POOL >> $LOG
cat $LOG >> $LOG_TEMP
expect_normal_exit ./libpmempool_test$EXESUFFIX -s 999999 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST2 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 0 -a 1 -r 0 -t obj $POOL>> $LOG

.


src/test/libpmempool_api/TEST3 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 1 -a 0 -r 0 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST4 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 1 -a 1 -r 0 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST5 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-d 1 -b $POOL.backup -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST6 line 26 at r1 (raw file):


expect_normal_exit ./libpmempool_test$EXESUFFIX\
	-s 0 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST7 line 25 at r1 (raw file):

expect_normal_exit $PMEMWRITE$EXESUFFIX $POOL TEST

expect_normal_exit ./libpmempool_test$EXESUFFIX -r 0 -y 1 -t obj $POOL >> $LOG

.


src/test/libpmempool_api/TEST8 line 6 at r1 (raw file):

#
#
# libpmempool_api/TEST8 -- test for checking API

Shall we keep this test for accessing obj pool with some unsupported pool type?


src/test/libpmempool_api/TEST9 line 6 at r1 (raw file):

#
#
# libpmempool_api/TEST9 -- test for checking API

What is the purpose of this test?

@janekmi janekmi force-pushed the remove_deprecated_pool_types_tests branch from b6a5f5e to e218e7e Compare June 27, 2023 09:16
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 31 of 31 files at r1, 23 of 23 files at r2, all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @grom72)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Do we plan to make another PR to remove all -t obj as we will support only one type of pool?
Or perhaps we need at first a PR that makes -t mandatory also for log tests?
libpmempool_test is used without -t only for libpmempool_api where we want to support only obj related tests. In the rest of tests -t is given explicitly - no risk of any side effects.

AFAIK after the change you have proposed most of the -t obj calls are gone. There are still a few in the libpmempool_backup directory. We can discuss it further when that directory will be cleaned up.



src/test/libpmempool_api/libpmempool_test.c line 105 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Can we assume that we support only one type of pool, the obj one?
In such case, we can skip -t obj in all tests.

Done.


src/test/libpmempool_api/out0.log.match line 8 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Should we not have here some obj-related messages?

Let me see. Here you can find check steps and which POOL_TYPES triggers each of them. You can find that _OBJ pools trigger the following check steps:

  • check_sds
  • check_pool_hdr
  • check_pool_hdr_uuids

The first two are already here. Whereas the third one seems not too verbose. Probably because no replicas are present in this test.

Ref:

for (; loc->replica < nreplicas; loc->replica++) {


src/test/libpmempool_api/out1.log.match line 8 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

.


src/test/libpmempool_api/out10.log.match line 16 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

What is the value of this test if it can not complete w/ obj correctly?

I decided to repurpose this test so we know that the fuse works correctly.


src/test/libpmempool_api/out13.log.match line 12 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

.


src/test/libpmempool_api/out8.log.match line 3 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Shall we keep this test with some random - non 'obj' value?

I would say no. Without the deprecated pool types, we end up with two at hand:

  • _DETECT and
  • _OBJ.

Where _DETECT cannot trigger this kind of mismatch error.

This reminds me we would have to implement (or adjust) the check detecting the reserved values. I guess there is no test for this use case right now.


src/test/libpmempool_api/out9.log.match line 8 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

What is the purpose of this test?

I would say there are two:

  • checking a certain pool header corruption and
  • attempt unsupported fix in this certain corruption case.

src/test/libpmempool_api/TEST0 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Would it be better to change default -t value from log -> obj?

Done.


src/test/libpmempool_api/TEST1 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST10 line 28 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST10 line 37 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST13 line 25 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST13 line 27 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST2 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST3 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST4 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST5 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST6 line 26 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST7 line 25 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/TEST8 line 6 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Shall we keep this test for accessing obj pool with some unsupported pool type?

There remains none to choose from.


src/test/libpmempool_api/TEST9 line 6 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

What is the purpose of this test?

As I said in the previous comment + this test also checks whether there is no difference between the pool prior to and after the repair attempt. So we can check whether the pool is not actually modified by unseccesfull repair.


src/test/libpmempool_api/out4.log.match line 2 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/out5.log.match line 2 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/out6.log.match line 2 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.


src/test/libpmempool_api/out7.log.match line 2 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

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 2 of 23 files at r2.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

AFAIK after the change you have proposed most of the -t obj calls are gone. There are still a few in the libpmempool_backup directory. We can discuss it further when that directory will be cleaned up.

Yes, but they are used explicitly there, so it should not be a problem.



src/test/libpmempool_api/out8.log.match line 3 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I would say no. Without the deprecated pool types, we end up with two at hand:

  • _DETECT and
  • _OBJ.

Where _DETECT cannot trigger this kind of mismatch error.

This reminds me we would have to implement (or adjust) the check detecting the reserved values. I guess there is no test for this use case right now.

Please create an issue for that.

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 23 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)


src/test/libpmempool_api/TEST8 line 6 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

There remains none to choose from.

But there will be some values in pmempool_pool_type after we remove everything.

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 20 of 23 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

@janekmi janekmi force-pushed the remove_deprecated_pool_types_tests branch from e218e7e to 9f38ac5 Compare June 27, 2023 12:47
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.

Reviewed 17 of 23 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

@janekmi janekmi force-pushed the remove_deprecated_pool_types_tests branch from 9f38ac5 to 627e970 Compare June 27, 2023 12: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 all commit messages.
Reviewable status: 31 of 32 files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Yes, but they are used explicitly there, so it should not be a problem.

Done.



src/test/libpmempool_api/out8.log.match line 3 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Please create an issue for that.

#5754


src/test/libpmempool_api/TEST8 line 6 at r1 (raw file):

This reminds me we would have to implement (or adjust) the check detecting the reserved values. I guess there is no test for this use case right now.

Maybe this will be the test which is missing. #5754

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:

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

@janekmi janekmi merged commit b95f2a1 into pmem:master Jun 27, 2023
- drop tests if log, blk and btt specific
- replace log, blk and btt using tests with obj tests
- pmemwrite has been extended to support PMEMOBJ pools as well

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
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