* [PATCH nft 1/3] tests: shell: README: copy edit @ 2021-10-20 12:45 Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 2/3] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Štěpán Němec @ 2021-10-20 12:45 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/shell/README b/tests/shell/README index e0279bbdc30c..35f6e3785f0e 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -1,16 +1,17 @@ -This test-suite is intended to perform tests of higher level than -the other regression test-suite. +This test suite is intended to perform tests on a higher level +than the other regression test suites. -It can run arbitrary executables which can perform any test apart of testing -the nft syntax or netlink code (which is what the regression tests does). +It can run arbitrary executables which can perform any test, not +limited to testing the nft syntax or netlink code (which is what +the regression tests do). To run the test suite (as root): $ cd tests/shell # ./run-tests.sh -Test files are executables files with the pattern <<name_N>>, where N is the -expected return code of the executable. Since they are located with `find', -test-files can be spread in any sub-directories. +Test files are executable files matching the pattern <<name_N>>, +where N is the expected return code of the executable. Since they +are located with `find', test files can be put in any subdirectory. You can turn on a verbose execution by calling: # ./run-tests.sh -v @@ -18,11 +19,11 @@ You can turn on a verbose execution by calling: And generate missing dump files with: # ./run-tests.sh -g <TESTFILE> -Before each call to the test-files, `nft flush ruleset' will be called. -Also, test-files will receive the environment variable $NFT which contains the -path to the nftables binary being tested. +Before each test file invocation, `nft flush ruleset' will be called. +Also, test file process environment will include the variable $NFT +which contains the path to the nft binary being tested. You can pass an arbitrary $NFT value as well: # NFT=/usr/local/sbin/nft ./run-tests.sh -By default the tests are run with the nft binary at '../../src/nft' +By default, the tests are run with the nft binary at '../../src/nft' base-commit: 2139913694a9850c9160920b2c638aac4828f9bb -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH nft 2/3] tests: shell: README: $NFT does not have to be a path to a binary 2021-10-20 12:45 [PATCH nft 1/3] tests: shell: README: copy edit Štěpán Němec @ 2021-10-20 12:45 ` Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 3/3] tests: shell: README: clarify test file name convention Štěpán Němec 2021-10-20 15:04 ` [PATCH nft 1/3] tests: shell: README: copy edit Phil Sutter 2 siblings, 0 replies; 17+ messages in thread From: Štěpán Němec @ 2021-10-20 12:45 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso Fixes: 7c8a44b25c22 ("tests: shell: Allow wrappers to be passed as nft command") Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/shell/README b/tests/shell/README index 35f6e3785f0e..4dd595d99556 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -21,7 +21,7 @@ And generate missing dump files with: Before each test file invocation, `nft flush ruleset' will be called. Also, test file process environment will include the variable $NFT -which contains the path to the nft binary being tested. +which contains the nft command being tested. You can pass an arbitrary $NFT value as well: # NFT=/usr/local/sbin/nft ./run-tests.sh -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH nft 3/3] tests: shell: README: clarify test file name convention 2021-10-20 12:45 [PATCH nft 1/3] tests: shell: README: copy edit Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 2/3] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec @ 2021-10-20 12:45 ` Štěpán Němec 2021-10-20 15:04 ` [PATCH nft 1/3] tests: shell: README: copy edit Phil Sutter 2 siblings, 0 replies; 17+ messages in thread From: Štěpán Němec @ 2021-10-20 12:45 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso Fixes: 4d26b6dd3c4c ("tests: shell: change all test scripts to return 0") Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/shell/README b/tests/shell/README index 4dd595d99556..07d5cc2e3e7c 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -10,8 +10,12 @@ To run the test suite (as root): # ./run-tests.sh Test files are executable files matching the pattern <<name_N>>, -where N is the expected return code of the executable. Since they -are located with `find', test files can be put in any subdirectory. +where N used to be the expected return code of the executable. +(After commit 4d26b6dd3c4c, all tests should return 0 on success, +no matter the test file name.) + +Since they are located with `find', test files can be put in any +subdirectory. You can turn on a verbose execution by calling: # ./run-tests.sh -v -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-20 12:45 [PATCH nft 1/3] tests: shell: README: copy edit Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 2/3] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 3/3] tests: shell: README: clarify test file name convention Štěpán Němec @ 2021-10-20 15:04 ` Phil Sutter 2021-10-21 8:30 ` Štěpán Němec 2 siblings, 1 reply; 17+ messages in thread From: Phil Sutter @ 2021-10-20 15:04 UTC (permalink / raw) To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso Hi, What you you mean with 'copy edit'? Please make subject line a bit more comprehensible. Also, adding at least a single line of description is mandatory, even if the subject speaks for itself. The actual changes look good to me, though! Thanks, Phil ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-20 15:04 ` [PATCH nft 1/3] tests: shell: README: copy edit Phil Sutter @ 2021-10-21 8:30 ` Štěpán Němec 2021-10-21 10:26 ` Phil Sutter 0 siblings, 1 reply; 17+ messages in thread From: Štěpán Němec @ 2021-10-21 8:30 UTC (permalink / raw) To: Phil Sutter; +Cc: netfilter-devel, Pablo Neira Ayuso [-- Attachment #1: Type: text/plain, Size: 1374 bytes --] On Wed, 20 Oct 2021 17:04:02 +0200 Phil Sutter wrote: > What you you mean with 'copy edit'? https://en.wikipedia.org/wiki/Copy_editing > Please make subject line a bit more comprehensible. Would "fix language issues" work for you? Or, could we perhaps keep the original subject, and add "fix language issues" in the body, to address your other concern? > Also, adding at least a single line of description is mandatory, even > if the subject speaks for itself. Commit messages consisting of only the subject and trailers (SOB et al.) are quite common, both in this project and elsewhere. [1] I suppose that as someone responsible for reviewing and applying patches, you're free to install any hoops you see fit for contributors to jump through, but if it is the project's and contributors' best interest you have in mind, I think it would be better if you mentioned the "description is always mandatory" rule explicitly in patch submission guidelines [2] (providing a rationale and being consistent about it in reality would be better still). Thanks, Štěpán [1] To get some picture, you can pipe output of the attached script to 'git log --stdin --no-walk', or further pipe that to 'git shortlog -ns' for a summary. [2] https://wiki.nftables.org/wiki-nftables/index.php/Portal:DeveloperDocs/Patch_submission_guidelines [-- Attachment #2: git_log_filter.perl --] [-- Type: text/plain, Size: 334 bytes --] #!/usr/bin/env perl use strict; use warnings; my $log_cmd = "git log -z --pretty=format:'%H\n%b'"; local $/ = "\0"; open(my $log_fh, '-|', $log_cmd) or die "Can't start git log: $!"; while (<$log_fh>) { chomp; my ($hash, $body) = split(/\n/, $_, 2); print($hash, "\n") if ($body =~ /\A\s*(?:[^:\n]+: +[^\n]+\n?)*\s*\Z/); } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-21 8:30 ` Štěpán Němec @ 2021-10-21 10:26 ` Phil Sutter 2021-10-21 11:03 ` Štěpán Němec 0 siblings, 1 reply; 17+ messages in thread From: Phil Sutter @ 2021-10-21 10:26 UTC (permalink / raw) To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso Hi, On Thu, Oct 21, 2021 at 10:30:25AM +0200, Štěpán Němec wrote: > On Wed, 20 Oct 2021 17:04:02 +0200 > Phil Sutter wrote: > > > What you you mean with 'copy edit'? > > https://en.wikipedia.org/wiki/Copy_editing Ah, thanks! I didn't know the term. > > Please make subject line a bit more comprehensible. > > Would "fix language issues" work for you? Or, could we perhaps keep the > original subject, and add "fix language issues" in the body, to address > your other concern? Yes, I'm fine with keeping the subject if a description is added. > > Also, adding at least a single line of description is mandatory, even > > if the subject speaks for itself. > > Commit messages consisting of only the subject and trailers (SOB et al.) > are quite common, both in this project and elsewhere. [1] Well yes, roughly 10% of all commits in nftables repo are. In iptables repo it's even worse with close to 50%. Thanks a lot for providing the script, BTW! :) > I suppose that as someone responsible for reviewing and applying > patches, you're free to install any hoops you see fit for contributors > to jump through, but if it is the project's and contributors' best > interest you have in mind, I think it would be better if you mentioned > the "description is always mandatory" rule explicitly in patch > submission guidelines [2] (providing a rationale and being consistent > about it in reality would be better still). Sorry for not checking the guideline but quoting advice I once received from the top of my head instead. Maybe I was incorrect and in obvious cases the description is optional. The relevant text in [2] at least doesn't explicitly state it is, while being pretty verbose about it regarding cover letters. Cheers, Phil > [2] https://wiki.nftables.org/wiki-nftables/index.php/Portal:DeveloperDocs/Patch_submission_guidelines ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-21 10:26 ` Phil Sutter @ 2021-10-21 11:03 ` Štěpán Němec 2021-10-27 9:06 ` Pablo Neira Ayuso 0 siblings, 1 reply; 17+ messages in thread From: Štěpán Němec @ 2021-10-21 11:03 UTC (permalink / raw) To: Phil Sutter; +Cc: netfilter-devel, Pablo Neira Ayuso On Thu, 21 Oct 2021 12:26:44 +0200 Phil Sutter wrote: > Sorry for not checking the guideline but quoting advice I once received > from the top of my head instead. Maybe I was incorrect and in obvious > cases the description is optional. The relevant text in [2] at least > doesn't explicitly state it is, while being pretty verbose about it > regarding cover letters. Does this mean that you retract your requirement? If not, could you please make sure that you and the other maintainers are on the same page about this, and document the requirement? Regarding this patch series (if it is to be resent, in part or as a whole): we've discussed the first patch so far; the other two patches have no description, either. Do they need one? If so, could you provide some suggestions? I can't think of anything sensible that isn't already said in the subject, Fixes:, or the modified README text itself. Thanks, Štěpán ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-21 11:03 ` Štěpán Němec @ 2021-10-27 9:06 ` Pablo Neira Ayuso 2021-10-27 9:51 ` Štěpán Němec 0 siblings, 1 reply; 17+ messages in thread From: Pablo Neira Ayuso @ 2021-10-27 9:06 UTC (permalink / raw) To: Štěpán Němec; +Cc: Phil Sutter, netfilter-devel On Thu, Oct 21, 2021 at 01:03:23PM +0200, Štěpán Němec wrote: > On Thu, 21 Oct 2021 12:26:44 +0200 > Phil Sutter wrote: > > > Sorry for not checking the guideline but quoting advice I once received > > from the top of my head instead. Maybe I was incorrect and in obvious > > cases the description is optional. The relevant text in [2] at least > > doesn't explicitly state it is, while being pretty verbose about it > > regarding cover letters. > > Does this mean that you retract your requirement? If not, could you > please make sure that you and the other maintainers are on the same page > about this, and document the requirement? > > Regarding this patch series (if it is to be resent, in part or as a > whole): we've discussed the first patch so far; the other two patches > have no description, either. Do they need one? If so, could you provide > some suggestions? I can't think of anything sensible that isn't already > said in the subject, Fixes:, or the modified README text itself. I also prefer if there is oneline description in the patch. My suggestions: * patch 1/3, not clear to me what "copy edit" means either. * patch 2/3, what is the intention there? a path to the nft executable is most generic way to refer how you use $NFT, right? * patch 3/3, I'd add a terse sentence so I do not need to scroll down and read the update to README to understand what this patch updates. I'd suggest: "Test files are located with find, so they can be placed in any location." Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this to the README file. The test infrastructure is only used for internal development use, this is included in tarballs but distributors do not package this. So please address Phil's feedback. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-27 9:06 ` Pablo Neira Ayuso @ 2021-10-27 9:51 ` Štěpán Němec 2021-10-27 10:13 ` Pablo Neira Ayuso 0 siblings, 1 reply; 17+ messages in thread From: Štěpán Němec @ 2021-10-27 9:51 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel On Wed, 27 Oct 2021 11:06:00 +0200 Pablo Neira Ayuso wrote: > I also prefer if there is oneline description in the patch. My > suggestions: > > * patch 1/3, not clear to me what "copy edit" means either. Description proposal: Grammar, wording, formatting fixes (no substantial change of meaning). > * patch 2/3, what is the intention there? a path to the nft executable > is most generic way to refer how you use $NFT, right? No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly or at least also meant for humans, i.e., that the person reading the commit message and wanting to better understand the change would look at the referenced commit, but if that is a wrong assumption to make, I propose to add the following description: Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. 'valgrind nft'. > * patch 3/3, I'd add a terse sentence so I do not need to scroll down > and read the update to README to understand what this patch updates. > I'd suggest: "Test files are located with find, so they can be placed > in any location." That text was just split to a separate paragraph, it has nothing to do with the actual change. > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this > to the README file. The test infrastructure is only used for internal > development use, this is included in tarballs but distributors do not > package this. IMO this argument should speak _for_ including the commit reference rather than omitting it, as the developer is more likely to be interested in the commit than the consumer. I thought about making the wording simply describe the current state without any historical explanations, but saying "Test files are executable files matching the pattern <<name_N>>, where N doesn't mean anything." seemed weird. Please advise. -- Štěpán ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-27 9:51 ` Štěpán Němec @ 2021-10-27 10:13 ` Pablo Neira Ayuso 2021-10-27 11:04 ` Štěpán Němec 0 siblings, 1 reply; 17+ messages in thread From: Pablo Neira Ayuso @ 2021-10-27 10:13 UTC (permalink / raw) To: Štěpán Němec; +Cc: Phil Sutter, netfilter-devel On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote: > On Wed, 27 Oct 2021 11:06:00 +0200 > Pablo Neira Ayuso wrote: > > > I also prefer if there is oneline description in the patch. My > > suggestions: > > > > * patch 1/3, not clear to me what "copy edit" means either. > > Description proposal: > > Grammar, wording, formatting fixes (no substantial change of meaning). > > > * patch 2/3, what is the intention there? a path to the nft executable > > is most generic way to refer how you use $NFT, right? > > No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly > or at least also meant for humans, i.e., that the person reading the > commit message and wanting to better understand the change would look at > the referenced commit, but if that is a wrong assumption to make, I > propose to add the following description: > Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. > 'valgrind nft'. OK, but why the reader need to know about former behaviour? The git repository already provides the historical information if this is of his interest. To me, the README file should contain the most up to date information that is relevant to run the test infrastructure. > > * patch 3/3, I'd add a terse sentence so I do not need to scroll down > > and read the update to README to understand what this patch updates. > > I'd suggest: "Test files are located with find, so they can be placed > > in any location." > > That text was just split to a separate paragraph, it has nothing to do > with the actual change. OK, then please document every update in your patch. > > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this > > to the README file. The test infrastructure is only used for internal > > development use, this is included in tarballs but distributors do not > > package this. > > IMO this argument should speak _for_ including the commit reference > rather than omitting it, as the developer is more likely to be > interested in the commit than the consumer. > > I thought about making the wording simply describe the current state > without any historical explanations, but saying "Test files are > executable files matching the pattern <<name_N>>, where N doesn't mean > anything." seemed weird. For historical explanations, you can dig into git. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-27 10:13 ` Pablo Neira Ayuso @ 2021-10-27 11:04 ` Štěpán Němec 2021-10-27 19:07 ` Pablo Neira Ayuso 0 siblings, 1 reply; 17+ messages in thread From: Štěpán Němec @ 2021-10-27 11:04 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel On Wed, 27 Oct 2021 12:13:57 +0200 Pablo Neira Ayuso wrote: > On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote: >> > * patch 2/3, what is the intention there? a path to the nft executable >> > is most generic way to refer how you use $NFT, right? >> >> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly >> or at least also meant for humans, i.e., that the person reading the >> commit message and wanting to better understand the change would look at >> the referenced commit, but if that is a wrong assumption to make, I >> propose to add the following description: > >> Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. >> 'valgrind nft'. > > OK, but why the reader need to know about former behaviour? The git > repository already provides the historical information if this is of > his interest. To me, the README file should contain the most up to > date information that is relevant to run the test infrastructure. I agree, and that's what the patch does: 'a path to the nft executable' is the former behaviour, arbitrary command is the most up to date information. (The "Since..." text above is my proposal for the commit description, not for the README file.) >> > * patch 3/3, I'd add a terse sentence so I do not need to scroll down >> > and read the update to README to understand what this patch updates. >> > I'd suggest: "Test files are located with find, so they can be placed >> > in any location." >> >> That text was just split to a separate paragraph, it has nothing to do >> with the actual change. > > OK, then please document every update in your patch. >> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this >> > to the README file. The test infrastructure is only used for internal >> > development use, this is included in tarballs but distributors do not >> > package this. >> >> IMO this argument should speak _for_ including the commit reference >> rather than omitting it, as the developer is more likely to be >> interested in the commit than the consumer. >> >> I thought about making the wording simply describe the current state >> without any historical explanations, but saying "Test files are >> executable files matching the pattern <<name_N>>, where N doesn't mean >> anything." seemed weird. > > For historical explanations, you can dig into git. Would the following README text work for you? Test files are executable files matching the pattern <<name_N>>, where N has no meaning. All tests should return 0 on success. Since they are located with `find', test files can be put in any subdirectory. Maybe saying "where N should be 0 in all new tests" would be less strange? I think both are significantly less helpful than my original version, but at least they aren't wrong like the current text. Proposed commit description, based on your comments: Since commit 4d26b6dd3c4c, test file name suffix no longer reflects expected exit code in all cases. Move the sentence "Since they are located with `find', test files can be put in any subdirectory." to a separate paragraph. -- Štěpán ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH nft 1/3] tests: shell: README: copy edit 2021-10-27 11:04 ` Štěpán Němec @ 2021-10-27 19:07 ` Pablo Neira Ayuso 2021-11-05 11:39 ` [PATCH nft v2 1/4] " Štěpán Němec 0 siblings, 1 reply; 17+ messages in thread From: Pablo Neira Ayuso @ 2021-10-27 19:07 UTC (permalink / raw) To: Štěpán Němec; +Cc: Phil Sutter, netfilter-devel On Wed, Oct 27, 2021 at 01:04:38PM +0200, Štěpán Němec wrote: > On Wed, 27 Oct 2021 12:13:57 +0200 > Pablo Neira Ayuso wrote: > > > On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote: > >> > * patch 2/3, what is the intention there? a path to the nft executable > >> > is most generic way to refer how you use $NFT, right? > >> > >> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly > >> or at least also meant for humans, i.e., that the person reading the > >> commit message and wanting to better understand the change would look at > >> the referenced commit, but if that is a wrong assumption to make, I > >> propose to add the following description: > > > >> Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. > >> 'valgrind nft'. > > > > OK, but why the reader need to know about former behaviour? The git > > repository already provides the historical information if this is of > > his interest. To me, the README file should contain the most up to > > date information that is relevant to run the test infrastructure. > > I agree, and that's what the patch does: 'a path to the nft executable' > is the former behaviour, arbitrary command is the most up to date > information. (The "Since..." text above is my proposal for the commit > description, not for the README file.) OK, this slightly more verbose description in the paragraph above LGTM. [...] > >> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this > >> > to the README file. The test infrastructure is only used for internal > >> > development use, this is included in tarballs but distributors do not > >> > package this. > >> > >> IMO this argument should speak _for_ including the commit reference > >> rather than omitting it, as the developer is more likely to be > >> interested in the commit than the consumer. > >> > >> I thought about making the wording simply describe the current state > >> without any historical explanations, but saying "Test files are > >> executable files matching the pattern <<name_N>>, where N doesn't mean > >> anything." seemed weird. > > > > For historical explanations, you can dig into git. > > Would the following README text work for you? > > Test files are executable files matching the pattern <<name_N>>, > where N has no meaning. All tests should return 0 on success. > > Since they are located with `find', test files can be put in any > subdirectory. LGTM. > Maybe saying "where N should be 0 in all new tests" would be less > strange? This is also OK, we can probably remove the trailing 0 at some point given it has no meaning anymore. > I think both are significantly less helpful than my original version, > but at least they aren't wrong like the current text. Makes sense. > Proposed commit description, based on your comments: > > Since commit 4d26b6dd3c4c, test file name suffix no longer reflects > expected exit code in all cases. > > Move the sentence "Since they are located with `find', test files can > be put in any subdirectory." to a separate paragraph. LGTM. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH nft v2 1/4] tests: shell: README: copy edit 2021-10-27 19:07 ` Pablo Neira Ayuso @ 2021-11-05 11:39 ` Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 2/4] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Štěpán Němec @ 2021-11-05 11:39 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso; +Cc: phil Grammar, wording, formatting fixes (no substantial change of meaning). Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/shell/README b/tests/shell/README index e0279bbdc30c..35f6e3785f0e 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -1,16 +1,17 @@ -This test-suite is intended to perform tests of higher level than -the other regression test-suite. +This test suite is intended to perform tests on a higher level +than the other regression test suites. -It can run arbitrary executables which can perform any test apart of testing -the nft syntax or netlink code (which is what the regression tests does). +It can run arbitrary executables which can perform any test, not +limited to testing the nft syntax or netlink code (which is what +the regression tests do). To run the test suite (as root): $ cd tests/shell # ./run-tests.sh -Test files are executables files with the pattern <<name_N>>, where N is the -expected return code of the executable. Since they are located with `find', -test-files can be spread in any sub-directories. +Test files are executable files matching the pattern <<name_N>>, +where N is the expected return code of the executable. Since they +are located with `find', test files can be put in any subdirectory. You can turn on a verbose execution by calling: # ./run-tests.sh -v @@ -18,11 +19,11 @@ You can turn on a verbose execution by calling: And generate missing dump files with: # ./run-tests.sh -g <TESTFILE> -Before each call to the test-files, `nft flush ruleset' will be called. -Also, test-files will receive the environment variable $NFT which contains the -path to the nftables binary being tested. +Before each test file invocation, `nft flush ruleset' will be called. +Also, test file process environment will include the variable $NFT +which contains the path to the nft binary being tested. You can pass an arbitrary $NFT value as well: # NFT=/usr/local/sbin/nft ./run-tests.sh -By default the tests are run with the nft binary at '../../src/nft' +By default, the tests are run with the nft binary at '../../src/nft' base-commit: 6ad2058da66affc105d325e45ff82fd5b5cac41e -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH nft v2 2/4] tests: shell: README: $NFT does not have to be a path to a binary 2021-11-05 11:39 ` [PATCH nft v2 1/4] " Štěpán Němec @ 2021-11-05 11:39 ` Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 3/4] tests: shell: README: clarify test file name convention Štěpán Němec ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Štěpán Němec @ 2021-11-05 11:39 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso; +Cc: phil Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. 'valgrind nft'. Fixes: 7c8a44b25c22 ("tests: shell: Allow wrappers to be passed as nft command") Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/shell/README b/tests/shell/README index 35f6e3785f0e..4dd595d99556 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -21,7 +21,7 @@ And generate missing dump files with: Before each test file invocation, `nft flush ruleset' will be called. Also, test file process environment will include the variable $NFT -which contains the path to the nft binary being tested. +which contains the nft command being tested. You can pass an arbitrary $NFT value as well: # NFT=/usr/local/sbin/nft ./run-tests.sh -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH nft v2 3/4] tests: shell: README: clarify test file name convention 2021-11-05 11:39 ` [PATCH nft v2 1/4] " Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 2/4] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec @ 2021-11-05 11:39 ` Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 4/4] tests: shell: $NFT needs to be invoked unquoted Štěpán Němec 2021-11-05 13:22 ` [PATCH nft v2 1/4] tests: shell: README: copy edit Phil Sutter 3 siblings, 0 replies; 17+ messages in thread From: Štěpán Němec @ 2021-11-05 11:39 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso; +Cc: phil Since commit 4d26b6dd3c4c, test file name suffix no longer reflects expected exit code in all cases. Move the sentence "Since they are located with `find', test files can be put in any subdirectory." to a separate paragraph. Fixes: 4d26b6dd3c4c ("tests: shell: change all test scripts to return 0") Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/shell/README b/tests/shell/README index 4dd595d99556..ea2b0b98f95f 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -10,8 +10,11 @@ To run the test suite (as root): # ./run-tests.sh Test files are executable files matching the pattern <<name_N>>, -where N is the expected return code of the executable. Since they -are located with `find', test files can be put in any subdirectory. +where N should be 0 in all new tests. All tests should return 0 on +success. + +Since they are located with `find', test files can be put in any +subdirectory. You can turn on a verbose execution by calling: # ./run-tests.sh -v -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH nft v2 4/4] tests: shell: $NFT needs to be invoked unquoted 2021-11-05 11:39 ` [PATCH nft v2 1/4] " Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 2/4] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 3/4] tests: shell: README: clarify test file name convention Štěpán Němec @ 2021-11-05 11:39 ` Štěpán Němec 2021-11-05 13:22 ` [PATCH nft v2 1/4] tests: shell: README: copy edit Phil Sutter 3 siblings, 0 replies; 17+ messages in thread From: Štěpán Němec @ 2021-11-05 11:39 UTC (permalink / raw) To: netfilter-devel, Pablo Neira Ayuso; +Cc: phil The variable has to undergo word splitting, otherwise the shell tries to find the variable value as an executable, which breaks in cases that 7c8a44b25c22 ("tests: shell: Allow wrappers to be passed as nft command") intends to support. Mention this in the shell tests README. Fixes: d8ccad2a2b73 ("tests: cover baecd1cf2685 ("segtree: Fix segfault when restoring a huge interval set")") Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 3 +++ tests/shell/testcases/sets/0068interval_stack_overflow_0 | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/shell/README b/tests/shell/README index ea2b0b98f95f..3af17a9e72ca 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -29,4 +29,7 @@ which contains the nft command being tested. You can pass an arbitrary $NFT value as well: # NFT=/usr/local/sbin/nft ./run-tests.sh +Note that, to support usage such as NFT='valgrind nft', tests must +invoke $NFT unquoted. + By default, the tests are run with the nft binary at '../../src/nft' diff --git a/tests/shell/testcases/sets/0068interval_stack_overflow_0 b/tests/shell/testcases/sets/0068interval_stack_overflow_0 index 134282de2826..6620572449c3 100755 --- a/tests/shell/testcases/sets/0068interval_stack_overflow_0 +++ b/tests/shell/testcases/sets/0068interval_stack_overflow_0 @@ -26,4 +26,4 @@ table inet test68_table { } EOF -( ulimit -s 128 && "$NFT" -f "$ruleset_file" ) +( ulimit -s 128 && $NFT -f "$ruleset_file" ) -- 2.33.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH nft v2 1/4] tests: shell: README: copy edit 2021-11-05 11:39 ` [PATCH nft v2 1/4] " Štěpán Němec ` (2 preceding siblings ...) 2021-11-05 11:39 ` [PATCH nft v2 4/4] tests: shell: $NFT needs to be invoked unquoted Štěpán Němec @ 2021-11-05 13:22 ` Phil Sutter 3 siblings, 0 replies; 17+ messages in thread From: Phil Sutter @ 2021-11-05 13:22 UTC (permalink / raw) To: Štěpán Němec; +Cc: netfilter-devel, Pablo Neira Ayuso On Fri, Nov 05, 2021 at 12:39:08PM +0100, Štěpán Němec wrote: > Grammar, wording, formatting fixes (no substantial change of meaning). Series applied, thanks for following-up! ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-11-05 13:22 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-20 12:45 [PATCH nft 1/3] tests: shell: README: copy edit Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 2/3] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec 2021-10-20 12:45 ` [PATCH nft 3/3] tests: shell: README: clarify test file name convention Štěpán Němec 2021-10-20 15:04 ` [PATCH nft 1/3] tests: shell: README: copy edit Phil Sutter 2021-10-21 8:30 ` Štěpán Němec 2021-10-21 10:26 ` Phil Sutter 2021-10-21 11:03 ` Štěpán Němec 2021-10-27 9:06 ` Pablo Neira Ayuso 2021-10-27 9:51 ` Štěpán Němec 2021-10-27 10:13 ` Pablo Neira Ayuso 2021-10-27 11:04 ` Štěpán Němec 2021-10-27 19:07 ` Pablo Neira Ayuso 2021-11-05 11:39 ` [PATCH nft v2 1/4] " Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 2/4] tests: shell: README: $NFT does not have to be a path to a binary Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 3/4] tests: shell: README: clarify test file name convention Štěpán Němec 2021-11-05 11:39 ` [PATCH nft v2 4/4] tests: shell: $NFT needs to be invoked unquoted Štěpán Němec 2021-11-05 13:22 ` [PATCH nft v2 1/4] tests: shell: README: copy edit Phil Sutter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.