ENT-12619: Reduced noise from cfbs shell test output

[larsewi]

Example output:

$ bash tests/shell/all.sh
Warning: These shell based tests use the cfbs you have installed
         If you haven't already, run: pip install .
--- PASS: 001_init (2s)
--- PASS: 002_add (2s)
--- PASS: 003_download (2s)
--- PASS: 004_build (12s)
--- PASS: 005_alias (3s)
--- PASS: 006_search (2s)
--- SKIP: 007_install (0s)
--- PASS: 008_remove (2s)
 ---snip---
--- PASS: 047_absolute_path_modules (4s)

==============================
Test Results: 46 passed, 0 failed, 1 skipped (total: 47, 156s)
==============================
All cfbs shell tests completed successfully!

[larsewi]

@cf-bottom Jenkins please :)

[cf-bottom]

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/13396/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-13396/

[craigcomstock]

I can't say I love this PR. There is a lot of change and now the scripts can't be written free-hand in bash... you must refer to the testlib often to know what functions to use.

Also, I am not sure how much is improved other than maybe silencing "good" outputs to make analysis easier... which is a welcome change so +1 on that account.

I think all the migrated commits could be squashed into one for easier review... #next-time

[larsewi]

@craigcomstock Thanks for the review 🚀

I can't say I love this PR. There is a lot of change and now the scripts can't be written free-hand in bash... you must refer to the testlib often to know what functions to use.

I see your point, but I disagree to some extent. The scripts can still be written in free-hand bash. The tests still worked previous to the migration. The asserts however are easier to understand for someone who is not very proficient with bash, rather than greps and pipes on top of each other. Furthermore, the asserts print out a nice error messages like expected foo, got bar, rather than test failing because grep returned 1.

I think all the migrated commits could be squashed into one for easier review... #next-time

For me, personally, it's easier with smaller commits, rather than 600 lines of diff that I have to scroll through. You can always click the files changed tab to see everything together.

I will fix up some things. If you still think this PR is bad, I won't die on my sword.

[olehermanse]

@larsewi Some notes:

• It looks like you're linking to the wrong ticket, ENT-13699 is about something else.
• As Craig notes, this goes a bit against what was the intention / advantage of these tests, being "just a bash script". Now, in order to write one "correctly" / consistently, you will have to check with what is in testlib frequently. I know you said you can still write them like before, but we shouldn't, we should aim to make them consistent.
• I agree that it's probably easier (at least sometimes / for some people) to read and write if you're not good at bash. Might be harder in other cases if you're encountering issues with testlib.sh for example.
• It looks like the output is better than before 👍
• There is a big tradeoff when it comes to abstraction. For example in this case, many tests have abstracted away the 5 steps they used to set up with a 2 line "source" and "init". Before, you could see exactly what happens in setup, now it's an opaque thing you have to look up in testlib.sh. Of course the advantage is that it's a bit shorter, more standardized, less chance of error (maybe), but in this case it's questionable if it's worth it.
• Why use a custom made testlib instead of reusing something which exists? For example this looks very similar? https://bashunit.typeddevs.com/assertions#assert-file-exists I'm not saying it's 100% wrong, but did you consider the pros and cons here?

[larsewi]

@olehermanse, @craigcomstock removed the test framework and kept the code print nicer output. Let me know what you think 😉

[craigcomstock]

Pretty small change with some nice benefits.