lint: Make Travis linting output less verbose in case of no violations #14159

practicalswift commented at 8:47 AM on September 6, 2018: contributor

Make Travis linting output less verbose in case of no violations, and thereby significantly increase signal to noise :-)

Before:

$ .travis/lint_06_script.sh
src/crypto/ctaes in HEAD currently refers to tree 1b6c31139a71f80245c09597c343936a8e41d021
src/crypto/ctaes in HEAD was last updated in commit 8501bedd7508ac514385806e191aec21ee978891 (tree 1b6c31139a71f80245c09597c343936a8e41d021)
subtree commit 003a4acfc273932ab8c2e276cde3b4f3541012dd unavailable: cannot compare
src/secp256k1 in HEAD currently refers to tree af619602c243e0d8fbd5934f375faa4aedb4ca6e
src/secp256k1 in HEAD was last updated in commit fd86f998fcfd25d823d67a2920814e22445655f9 (tree af619602c243e0d8fbd5934f375faa4aedb4ca6e)
subtree commit 0b7024185045a49a1a6a4c5615bf31c94f63d9c4 unavailable: cannot compare
src/univalue in HEAD currently refers to tree a2dbaf30021c0a31eecc213cd84d600273d333b6
src/univalue in HEAD was last updated in commit a570098021be6a7b9f4589300ea655ae4633628e (tree a2dbaf30021c0a31eecc213cd84d600273d333b6)
subtree commit 51d3ab34ba2857f0d03dc07250cb4a2b5e712e67 unavailable: cannot compare
src/leveldb in HEAD currently refers to tree f8cdc77167be86194f98d0412baf4b89d3d5abe6
src/leveldb in HEAD was last updated in commit ec749b1bcdf2483b642fb51d635800e272c68ba6 (tree f8cdc77167be86194f98d0412baf4b89d3d5abe6)
subtree commit 524b7e36a8e3bce6fcbcd1b5df09024283f325ba unavailable: cannot compare
Args used        : 181
Args documented  : 186
Args undocumented: 0
set()
Args unknown     : 5
{'-zmqpubrawtx', '-zmqpubhashtx', '-zmqpubhashblock', '-zmqpubrawblock', '-promiscuousmempoolflags'}
* Checking consistency between dispatch tables and vRPCConvertParams
$

After:

$ .travis/lint_06_script.sh
The following args are unknown:
* -promiscuousmempoolflags
* -zmqpubhashblock
* -zmqpubhashtx
* -zmqpubrawblock
* -zmqpubrawtx
$

fanquake added the label Tests on Sep 6, 2018

practicalswift force-pushed on Sep 6, 2018

unknown approved

scravy commented at 12:59 PM on September 6, 2018: contributor

utACK fc28c6b04acd5880ff821766d15c9b47fc5699ae

scravy approved

practicalswift force-pushed on Sep 7, 2018

lint: Make Travis linting output less verbose in case of no violations bd81fd42a8

practicalswift force-pushed on Sep 7, 2018

conscott commented at 7:57 PM on September 16, 2018: contributor

Nice. Tested ACK bd81fd42a88df462ccbf75e865138d17250a293b

ken2812221 commented at 11:45 PM on September 16, 2018: contributor

NACK. I don't think this is worth doing this. It's only 19 lines.

practicalswift commented at 5:59 AM on September 17, 2018: contributor

@ken2812221 The problem with the 19 lines is not only the number of lines but the fact that it looks like a lot of errors occurred when everything worked as expected :-) That is poor usability.

ken2812221 commented at 6:24 AM on September 17, 2018: contributor

@practicalswift In my opinion, I would prefer folding them instead of hiding them. This would make it less noise, too. If I want to know additional information, I can unfold them.

scravy commented at 10:01 AM on September 19, 2018: contributor

I like it a lot better how it is present in this PR. These lines are never relevant and they actually do look like errors. If the subtree check actually fails it will still print the failure. If it doesn't, then it's not of interest as it passed.

I do think any linter/check should not say anything unless it actually found a problem.

scravy commented at 2:59 PM on September 19, 2018: contributor

tested ACK bd81fd42a88df462ccbf75e865138d17250a293b

checked out the branch, ./travis/lint_06_script.sh returned the output as advertised with exit code 0. corrupted a subtree, committed, and ran the lint script again, fails properly as it should with exit code 1 and all the output you were hoping for.

in test/lint/check-doc.py:42 in bd81fd42a8

  33 | @@ -34,15 +34,19 @@ def main():
  34 |      args_need_doc = args_used.difference(args_docd)
  35 |      args_unknown = args_docd.difference(args_used)
  36 |  
  37 | -    print("Args used        : {}".format(len(args_used)))
  38 | -    print("Args documented  : {}".format(len(args_docd)))
  39 | -    print("Args undocumented: {}".format(len(args_need_doc)))
  40 | -    print(args_need_doc)
  41 | -    print("Args unknown     : {}".format(len(args_unknown)))
  42 | -    print(args_unknown)

MarcoFalke commented at 3:20 PM on September 19, 2018:

My intention was to always print the results, so that errors in the script could be detected.

I think a better solution would be to append the following lines to each script:

exit_with_error = errors > 0 or len(args_need_doc) or whatever
print('ERROR' if exit_with_error else 'GOOD')
print()

This way, the output is preserved, but it is also easy to spot an error sys.exit(exit_with_error)

scravy commented at 3:24 PM on September 19, 2018:

but the proposed solution already prints the full output on non-zero exit code.

MarcoFalke commented at 4:04 PM on September 19, 2018:

A zero exit code does not prove the script is not erroneous. Only a non-zero exit code indicates something is faulty.

So my intention really was to always print the result.

MarcoFalke commented at 4:05 PM on September 19, 2018:

I am not going to look up every instance of this, but we had more than one issue where a test script was doing exit(0) and only doing that, not actually checking or running anything.

practicalswift commented at 3:00 PM on September 21, 2018:

@MarcoFalke I'm not sure I follow TBH. What conditions do you want to print and/or error on in check-doc.py?

MarcoFalke commented at 5:16 PM on September 21, 2018:

If you never print anything on zero-exit code, bugs such as this would potentially never be found: 136084470cd7920b94e016621ebf682996247162

MarcoFalke commented at 2:11 AM on October 4, 2018: member

There seems to be some disagreement here (two NACKs) without forward progess afterward. Closing for now.

MarcoFalke closed this on Oct 4, 2018

practicalswift commented at 8:11 AM on October 4, 2018: contributor

@MarcoFalke Please re-open – I'm still working on this and I've now addressed your concern :-)

MarcoFalke commented at 8:16 AM on October 4, 2018: member

GitHub has the button disabled label="The make-linting-output-less-verbose branch was force-pushed or recreated." disabled=""

practicalswift commented at 8:17 AM on October 4, 2018: contributor

@MarcoFalke Ah, I see. I'll simply open a new PR then :-)

MarcoFalke locked this on Sep 8, 2021