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: cleanup pmempool_[!check] from deprecated tests #5749

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Jun 26, 2023

This change is Reviewable

@janekmi janekmi added the sprint goal This pull request is part of the ongoing sprint label Jun 26, 2023
@janekmi janekmi added this to the 2.0.0 milestone Jun 26, 2023
@janekmi janekmi force-pushed the remove_deprecated_pmempool_generic branch from 086d996 to 29d1162 Compare June 26, 2023 13:21
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #5749 (c951317) into master (e2d0619) will decrease coverage by 1.42%.
The diff coverage is n/a.

❗ Current head c951317 differs from pull request most recent head 5e1e291. Consider uploading reports for the commit 5e1e291 to get more accurate results

@@            Coverage Diff             @@
##           master    #5749      +/-   ##
==========================================
- Coverage   72.35%   70.94%   -1.42%     
==========================================
  Files         143      143              
  Lines       21982    21985       +3     
  Branches     3697     3699       +2     
==========================================
- Hits        15906    15598     -308     
- Misses       6076     6387     +311     

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 31 of 31 files at r1, 2 of 36 files at r2.
Reviewable status: 33 of 67 files reviewed, 1 unresolved discussion (waiting on @janekmi)


src/test/pmempool_sync/TEST26 line 48 at r2 (raw file):


# Remove poolset
expect_normal_exit $PMEMPOOL$EXESUFFIX rm $POOLSET

What is the purpose of this new version of the test?

@janekmi janekmi force-pushed the remove_deprecated_pmempool_generic branch from 29d1162 to f92c69d Compare June 27, 2023 14:44
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: 8 of 68 files reviewed, 1 unresolved discussion (waiting on @grom72)


src/test/pmempool_sync/TEST26 line 48 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

What is the purpose of this new version of the test?

Exactly the same. As far as I can tell, this test checked exactly the same scenario for both obj and log pools.
I have just dropped the log version. It seems the obj version is still valid without it.

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 36 files at r2, 16 of 32 files at r3, all commit messages.
Reviewable status: 38 of 68 files reviewed, 3 unresolved discussions (waiting on @janekmi)


ChangeLog line 5 at r3 (raw file):

	This release:
	- removes libpmempool API tests that use log/blk/btt pool types (partially)
	- removes pmempool !check tests that use log/blk/btt pool types

Suggestion:

removes pmempool_check

src/test/pmempool_dump/out0.log.match line 2 at r3 (raw file):

TEST
TEST

Would it be possible to remove this file if it is empty?

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

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


src/test/pmempool_dump/out0.log.match line 2 at r3 (raw file):


It should be err0.log.match
image.png


src/test/pmempool_info/out27.log.match line 36 at r3 (raw file):

Heap size                : $(*)
Checksum                 : $(*) [OK]
Root offset              : $(*)

EOL


src/test/pmempool_rm/TEST0 line 18 at r3 (raw file):

rm -f $LOG && touch $LOG

# Create pmemobj

Suggestion:

Create pmemobj pool

@janekmi janekmi force-pushed the remove_deprecated_pmempool_generic branch from c2f2668 to c951317 Compare June 28, 2023 11:18
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: 63 of 68 files reviewed, 5 unresolved discussions (waiting on @grom72 and @osalyk)


ChangeLog line 5 at r3 (raw file):

	This release:
	- removes libpmempool API tests that use log/blk/btt pool types (partially)
	- removes pmempool !check tests that use log/blk/btt pool types

No. I mean it. This PR touches everything but pmempool_check. Hence the exclamation mark states the opposite of the check.


src/test/pmempool_dump/out0.log.match line 2 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Would it be possible to remove this file if it is empty?

Done.


src/test/pmempool_dump/out0.log.match line 2 at r3 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

It should be err0.log.match
image.png

Done.


src/test/pmempool_info/out27.log.match line 36 at r3 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

EOL

Done.


src/test/pmempool_rm/TEST0 line 18 at r3 (raw file):

rm -f $LOG && touch $LOG

# Create pmemobj

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.

:lgtm:

Reviewed 5 of 5 files at r4, 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, 3 unresolved discussions (waiting on @osalyk)

@janekmi janekmi force-pushed the remove_deprecated_pmempool_generic branch from c951317 to 5e1e291 Compare June 28, 2023 11:29
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 4 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit 34f14bb into pmem:master Jun 28, 2023
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