.tools: Fix redundant r.Pass check when 'fail == 0' #174

wking · 2015-09-10T18:44:06Z

The previous version of this script would never print the success
messages, because when fail == 0 was true, none of the messages
would satisfy !r.Pass. This commit shifts the logic around to handle
the following cases:

a. If the verbose flag is set, print messages for all checks, using
TAP's "ok" and "not ok" to distinguish between per-check pass/fail.
b. If the verbose flag is not set, only print the failed messages (but
still keep the "not ok" prefix).

Spun off from #170.

vbatts · 2015-09-10T19:02:37Z

i'm not seeing the brokenness?

vbatts@valse ~/src/opencontainers/specs (fix-dco-validate-on-merges) $ gr .tools/validate.go -range 92a7451...5a654b9
 * 5a654b9 .tools: cleanup the commit entry ... PASS
 * 041eb73 travis: fix DCO validation for merges ... PASS
 * 3f62423 Merge remote-tracking branch 'origin/pr/159' ... PASS
 * 712a746 Merge remote-tracking branch 'origin/pr/163' ... PASS
 * b1510a7 Merge pull request #172 from laijs/committer ... PASS
 * d3a6069 Change the rlimit type to string instead of int ... PASS
 * 339e038 Deduplicate the field of RootfsPropagation ... PASS
vbatts@valse ~/src/opencontainers/specs (fix-dco-validate-on-merges) $ gr .tools/validate.go -v -range 92a7451...5a654b9
 * 5a654b9 .tools: cleanup the commit entry ... PASS
  - has a valid DCO
 * 041eb73 travis: fix DCO validation for merges ... PASS
  - has a valid DCO
 * 3f62423 Merge remote-tracking branch 'origin/pr/159' ... PASS
  - merge commits do not require DCO
 * 712a746 Merge remote-tracking branch 'origin/pr/163' ... PASS
  - merge commits do not require DCO
 * b1510a7 Merge pull request #172 from laijs/committer ... PASS
  - merge commits do not require DCO
 * d3a6069 Change the rlimit type to string instead of int ... PASS
  - has a valid DCO
 * 339e038 Deduplicate the field of RootfsPropagation ... PASS
  - has a valid DCO

wking · 2015-09-10T19:18:08Z

On Thu, Sep 10, 2015 at 12:02:39PM -0700, Vincent Batts wrote:

i'm not seeing the brokenness?

Your post-#170 phrasing isn't broken, it's just redundant. Details in
my commit message (5282b96, rebased onto #170), but the redundancy is
(eliding some nesting):

if _, fail := vr.PassFail(); fail == 0 {
…
if r.Pass {
fmt.Printf(" - %s\n", r.Msg)
}
}

How would the internal if check ever not fire? That's why my
initial suggestion was to just drop the internal if, and use 1:

if _, fail := vr.PassFail(); fail == 0 {
…
fmt.Printf(" - %s\n", r.Msg)
}

This PR does more than just drop the check, but that's what I was
pushing back on initially.

vbatts · 2015-09-11T14:08:30Z

not. because that if r.Pass { is inside a loop for that validate set, not the single rule that was tested. A validation set may have failures and passes.

wking · 2015-09-11T14:35:21Z

On Fri, Sep 11, 2015 at 07:08:31AM -0700, Vincent Batts wrote:

not. because that if r.Pass { is inside a loop for that validate
set, not the single rule that was tested. A validation set may
have failures and passes.

In general, a set may have failures and passes, but your entire loop
is inside the if _, fail := vr.PassFail(); fail == 0 check, which
means that none of the checks in that set failed.

vbatts · 2015-09-11T15:28:27Z

i see. I'm wrong here. If pass count is 0 then there is no need for if r.Pass.

Let's remove this beginings of TAP output. It makes the layout not logical
and is not even proper TAP

On Fri, Sep 11, 2015 at 10:35 AM, W. Trevor King [email protected]
wrote:

On Fri, Sep 11, 2015 at 07:08:31AM -0700, Vincent Batts wrote:

not. because that if r.Pass { is inside a loop for that validate
set, not the single rule that was tested. A validation set may
have failures and passes.

In general, a set may have failures and passes, but your entire loop
is inside the if _, fail := vr.PassFail(); fail == 0 check, which
means that none of the checks in that set failed.

—
Reply to this email directly or view it on GitHub
#174 (comment).

wking · 2015-09-11T15:40:17Z

On Fri, Sep 11, 2015 at 08:28:29AM -0700, Vincent Batts wrote:

Let's remove this beginings of TAP output. It makes the layout not
logical and is not even proper TAP

Do you have a different preference for distinguishing passing checks
from fails (maybe + vs - where you always use -)? I'm fine just
converting the whole thing to valid TAP, but I'm also happy to punt
that to a follow-up PR.

The previous version of this script would always pass this check, because when 'fail == 0' was true, all of the messages had passed. That lead to logic like: a. If all checks passed for the commit and the verbose flag is set, print all the messages (which all passed). b. If all checks passed for the commit and the verbose flag is not set, don't print details about any messages (which all passed). c. If any checks failed, print all the failed messages. That wasn't printing successful checks when the verbose flag was true but some checks failed. This commit shifts the logic around to: A. If the verbose flag is set, print messages for all checks, using TAP's "ok" and "not ok" to distinguish between per-check pass/fail [1]. B. If the verbose flag is not set, only print the failed messages (but still keep the "not ok" prefix). In the non-verbose case, then (b/c) and (B) look the same (modulo my "not ok" prefix adjustment). And in the verbose case, I think (A) is preferable to (a/c), because there's no reason to *not* print successful checks when you've enabled the verbose flag. [1]: http://testanything.org/ Signed-off-by: W. Trevor King <[email protected]>

wking · 2015-09-12T04:12:19Z

On Fri, Sep 11, 2015 at 08:38:14AM -0700, W. Trevor King wrote:

Fri, Sep 11, 2015 at 08:28:29AM -0700, Vincent Batts:

Let's remove this beginings of TAP output. It makes the layout not
logical and is not even proper TAP

Do you have a different preference for distinguishing passing checks
from fails (maybe + vs - where you always use -)? I'm fine just
converting the whole thing to valid TAP, but I'm also happy to punt
that to a follow-up PR.

In the absence of feedback here, I've added a follow-up commit
(2d70cd9) that makes the verbose output valid TAP and avoids the
de-indenting for non-verbose output. I'm still happy to pull that
commit off into a follow-up PR and reroll my initial commit if you
tell me how you'd rather distingish betwee passing checks and fails.

wking · 2015-09-12T04:15:47Z

On Fri, Sep 11, 2015 at 09:10:13PM -0700, W. Trevor King wrote: … I've added a follow-up commit (2d70cd9)… Looks like that commit was missing a DCO (yay, CI :). Added with 2d70cd9 → 6beba35.

Using TAP [1] makes it easy to consume the output of this tool in a variety of existing harnesses [2]. Only the verbose results are TAP compliant, mostly because folks aren't likely to care about verbosity if they're just piping the results into a TAP harness. If we end up wanting the non-verbose output to be TAP compliant, we'll have to switch to the trailing count approach explained in the "Unknown amount and failures" section of the spec [1], and we'll have to keep a running counter of failed checks (instead of the current len(DefaultRules)-based approach). [1]: http://testanything.org/tap-version-13-specification.html [2]: http://testanything.org/consumers.html Signed-off-by: W. Trevor King <[email protected]>

wking · 2015-09-12T04:30:28Z

Fixed 'go fmt' with 6beba35 → 650828c.

crosbymichael · 2015-10-06T17:14:14Z

This was moved to a separate project out of this repo.

wking mentioned this pull request Sep 10, 2015

travis: fix DCO validation for merges #170

Merged

wking force-pushed the fail-zero-fixup branch from 7280f10 to 5282b96 Compare September 10, 2015 19:10

wking force-pushed the fail-zero-fixup branch from 5282b96 to 2d70cd9 Compare September 12, 2015 04:06

wking force-pushed the fail-zero-fixup branch from 2d70cd9 to 6beba35 Compare September 12, 2015 04:12

wking force-pushed the fail-zero-fixup branch from 6beba35 to 650828c Compare September 12, 2015 04:27

wking mentioned this pull request Sep 12, 2015

.tools: Add per-commit Go checks (vet, fmt, and lint) #182

Closed

wking changed the title ~~.tools: Fix broken !r.Pass check when 'fail == 0'~~ .tools: Fix redundant r.Pass check when 'fail == 0' Sep 17, 2015

crosbymichael closed this Oct 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

.tools: Fix redundant r.Pass check when 'fail == 0' #174

.tools: Fix redundant r.Pass check when 'fail == 0' #174

Uh oh!

wking commented Sep 10, 2015

Uh oh!

vbatts commented Sep 10, 2015

Uh oh!

wking commented Sep 10, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

wking commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

wking commented Sep 11, 2015

Uh oh!

wking commented Sep 12, 2015

Uh oh!

wking commented Sep 12, 2015 via email

Uh oh!

wking commented Sep 12, 2015 via email

Uh oh!

crosbymichael commented Oct 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

.tools: Fix redundant r.Pass check when 'fail == 0' #174

.tools: Fix redundant r.Pass check when 'fail == 0' #174

Uh oh!

Conversation

wking commented Sep 10, 2015

Uh oh!

vbatts commented Sep 10, 2015

Uh oh!

wking commented Sep 10, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

wking commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

wking commented Sep 11, 2015

Uh oh!

wking commented Sep 12, 2015

Uh oh!

wking commented Sep 12, 2015 via email

Uh oh!

wking commented Sep 12, 2015 via email

Uh oh!

crosbymichael commented Oct 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants