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

common: extend SKIPed tests description #6052

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Mar 12, 2024

This change is Reviewable

@grom72 grom72 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 12, 2024
@grom72 grom72 requested review from janekmi and osalyk March 12, 2024 18:22
@grom72 grom72 removed the sprint goal This pull request is part of the ongoing sprint label Mar 12, 2024
@grom72 grom72 added the obsolete-src Obsolete source code label Mar 13, 2024
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 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

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 r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @grom72)


src/test/RUNTESTS.sh line 279 at r1 (raw file):

		ttype=$(intersection "$_testtype" "$req_testtype" "short medium long")
		[ -z "$ttype" ] && {
			echo "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) test-type $testtype ($req_testtype required)"

I would say non of these is set in this context.

Code quote:

			echo "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) test-type $testtype ($req_testtype required)"

src/test/tools/anonymous_mmap/check_max_mmap.sh line 32 at r1 (raw file):

function msg_skip() {
	echo "0" > "$FILE_MAX_DAX_DEVICES"
	echo "$0: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM): $*"

.


src/test/unittest/unittest.sh line 849 at r1 (raw file):

	msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) required: overcommit_memory enabled and unlimited virtual memory"
	exit 0
}

It seems irrelevant now. Nobody uses it.


src/test/unittest/unittest.sh line 910 at r1 (raw file):

	msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM): Only supported on $1"
	exit 0
}

.


src/test/unittest/unittest.sh line 923 at r1 (raw file):

		fi
	done
}

.


src/test/unittest/unittest.sh line 1586 at r1 (raw file):

		exit 0
	fi
}

.

Copy link
Contributor Author

@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, 6 unresolved discussions (waiting on @janekmi)


src/test/RUNTESTS.sh line 279 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I would say non of these is set in this context.

See example below


src/test/tools/anonymous_mmap/check_max_mmap.sh line 32 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Old:

ctl_prefault/TEST0: SETUP (check/non-pmem/debug)
ctl_prefault/TEST0: SKIP: filesystem does not support fallocate

New:

ctl_prefault/TEST0: SETUP (check/non-pmem/debug)
ctl_prefault/TEST0: SKIP (check/non-pmem/debug): filesystem does not support fallocate

src/test/unittest/unittest.sh line 849 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems irrelevant now. Nobody uses it.

But it is not a part of this PR


src/test/unittest/unittest.sh line 910 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.


src/test/unittest/unittest.sh line 923 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

Copy link
Contributor Author

@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, 6 unresolved discussions (waiting on @janekmi)


src/test/unittest/unittest.sh line 1586 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

@janekmi janekmi added this to the 2.1.0 milestone Mar 14, 2024
@grom72 grom72 modified the milestones: 2.1.0, 2.1.1 Mar 27, 2024
@janekmi janekmi modified the milestones: 2.1.1, 2.1.0 Mar 27, 2024
@grom72 grom72 modified the milestones: 2.1.0, 2.x Mar 28, 2024
@grom72 grom72 added the sprint goal This pull request is part of the ongoing sprint label Sep 16, 2024
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.07%. Comparing base (9819db6) to head (451905f).
Report is 121 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6052   +/-   ##
=======================================
  Coverage   70.07%   70.07%           
=======================================
  Files         133      133           
  Lines       19563    19563           
  Branches     3261     3261           
=======================================
  Hits        13708    13708           
  Misses       5855     5855           

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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72)


src/test/RUNTESTS.sh line 279 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

See example below

I just uncommented the condition and the result looks as follows:

arch_flags/TEST0: SKIP (//) test-type check (all required)

Am I missing something?


src/test/tools/anonymous_mmap/check_max_mmap.sh line 32 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Old:

ctl_prefault/TEST0: SETUP (check/non-pmem/debug)
ctl_prefault/TEST0: SKIP: filesystem does not support fallocate

New:

ctl_prefault/TEST0: SETUP (check/non-pmem/debug)
ctl_prefault/TEST0: SKIP (check/non-pmem/debug): filesystem does not support fallocate
$ ./check_max_mmap.sh 
./check_max_mmap.sh: SKIP (//): DEVICE_DAX_PATH does not specify path to DAX device.

What am I missing?


src/test/unittest/unittest.sh line 285 at r1 (raw file):

		;;
	*)
		verbose_msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) fs-type $FS (not configured)"

Verified. It works.

arch_flags/TEST0: SKIP (check/none/debug) fs-type none (not configured)
arch_flags/TEST0: SKIP (check/none/nondebug) fs-type none (not configured)
arch_flags/TEST0: SKIP (check/pmem/debug) fs-type pmem (not configured)
arch_flags/TEST0: SKIP (check/pmem/nondebug) fs-type pmem (not configured)
arch_flags/TEST0: SKIP (check/non-pmem/debug) fs-type non-pmem (not configured)
arch_flags/TEST0: SKIP (check/non-pmem/nondebug) fs-type non-pmem (not configured)
arch_flags/TEST0: SKIP (check/pmem/debug) fs-type any (not configured)
arch_flags/TEST0: SKIP (check/pmem/nondebug) fs-type any (not configured)

src/test/unittest/unittest.sh line 843 at r1 (raw file):

function require_unlimited_vm() {
	if grep -q "^mmu[[:blank:]]*: sv39" /proc/cpuinfo; then
		msg "$UNITTEST_NAME: SKIP ($TEST/$REAL_FS/$BUILD$MCSTR$PROV$PM) required: 4+ level virtual memory"

Since all the extended messages use the same variables as the one used above, the skip messages ought to be properly generated also in case of using the same exact variables from the function calls. At least as long as nothing overwrites these variables.

Copy link
Contributor Author

@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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)


src/test/RUNTESTS.sh line 279 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I just uncommented the condition and the result looks as follows:

arch_flags/TEST0: SKIP (//) test-type check (all required)

Am I missing something?

So please do not uncomment ;) as this is not a part of this PR


src/test/tools/anonymous_mmap/check_max_mmap.sh line 32 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…
$ ./check_max_mmap.sh 
./check_max_mmap.sh: SKIP (//): DEVICE_DAX_PATH does not specify path to DAX device.

What am I missing?

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.

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

@grom72 grom72 force-pushed the skip-w-full-info branch 2 times, most recently from cec2fdd to 1caee2f Compare October 3, 2024 11:53
Copy link
Contributor Author

@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: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @janekmi and @osalyk)


src/test/RUNTESTS.sh line 279 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

So please do not uncomment ;) as this is not a part of this PR

Done

Copy link
Contributor Author

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

Copy link
Contributor Author

@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 r1, 1 of 1 files at r2.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @janekmi and @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.

:lgtm:

Reviewed 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (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 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

@janekmi janekmi merged commit c779994 into pmem:master Oct 3, 2024
8 checks passed
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 obsolete-src Obsolete source code 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