TypedDevs / bashunit

A simple testing library for bash scripts. Test your bash scripts in the fastest and simplest way, discover the most modern bash testing library.
https://bashunit.typeddevs.com
MIT License
270 stars 21 forks source link

Increase contrast of test results #245

Closed h0adp0re closed 4 months ago

h0adp0re commented 4 months ago

📚 Description

Improve test report contrast.

Before

Screenshot 2024-02-26 at 19 17 04

After

Screenshot 2024-02-26 at 19 37 38

🔖 Changes

✅ To-do list

h0adp0re commented 4 months ago

And another suggestion that I did not yet implement. What if the final result state had padding whitespace? I find it improves legibility even further. Screenshot 2024-02-26 at 21 56 13

And perhaps even a preceding newline? Screenshot 2024-02-26 at 22 01 21

Chemaclass commented 4 months ago

Thanks for your effort, @h0adp0re! Can you please take care of the broken CI steps?

h0adp0re commented 4 months ago

Thanks for the library! Sure, it's the printf variable warning, I'll just commit it right away then.

h0adp0re commented 4 months ago

And perhaps even a preceding newline? Screenshot 2024-02-26 at 22 01 21

I implemented this version with the latest commit.

antonio-gg-dev commented 4 months ago

More broken tests on the CI @h0adp0re 🙏🏻

h0adp0re commented 4 months ago

Hmm, I will have to think about this. I thought I mitigated the missing $TERM problem for tput here.

h0adp0re commented 4 months ago

@Chemaclass @antonio-gg-dev @khru I've reached a conclusion that with tput the snapshots will be environment-specific. I don't see a way around it, unless a decision is made that the snapshots won't contain any escape codes.

If no such decision can be made at this time, I will have to revert the migration to tput and only stick with the color and spacing changes.


Edit: on the one hand omitting escape codes from snapshots gives users the opportunity to use bashunit cross-environment — if their own scripts use tput, for example. My scripts do and I would have a problem with the snapshots if I were to start using another terminal environment.

antonio-gg-dev commented 4 months ago

My vote is for keeping the behavior of bashunit as consistent across environments as possible, so I would only remove the use of tput while maintaining the changes in colors and spacing as you indicate.

On the other hand, omitting the escape codes from the snapshots seems like an interesting feature, but verbosely, as an alternative type of snapshot assertion, I would always maintain that the default behavior includes them.

h0adp0re commented 4 months ago

Yes, everything you said makes sense. I am up for adding assert_match_snapshot_ignore_colors, if this feature would interest you. Thus, I see three options at the moment:

  1. I will revert this project back to hard-coded escape sequences, and:
    1. the PR will be done,
    2. add assert_match_snapshot_ignore_colors.
  2. I will add assert_match_snapshot_ignore_colors and implement it in the project's snapshot tests, making snapshots independent of colors.

My order of preference is 2, 1.2, 1.1.

antonio-gg-dev commented 4 months ago

Hi @h0adp0re !

For now, work on 1.i so we can close this PR and keep it more concise with a single improvement.

You can work on 1.ii next if you want, but as I said, we want bashunit to operate well-covered and consistent across different environments, so even with this new assertion, it's best if our acceptance tests don't use it.

If tput is so inconsistent across environments, to me this means discarding its use.

h0adp0re commented 4 months ago

Hey, thanks for confirming. I took a small break from this problem to process it properly and I agree, I will do that. Atomic changes are traceable and maintainable. And, in the end, all I wanted to achieve was the contrast change.

I might take on assert_match_snapshot_ignore_colors in the near future when I get to testing some colorful scripts.

And at the risk of sounding repetitive — I still do recommend using tput in any script that is expected to be run in any number of unknown shells. Its variability is a deliberate feature that ensures the correct escape sequences for each environment. But for general use, I have to admit, ESC[m is probably supported widely enough.

antonio-gg-dev commented 4 months ago

Thank you very much for understanding @h0adp0re , of course, take all the time you need and don't feel pressured to do it. If at any point you decide not to continue, just let us know and we will take care of it from there.

We greatly appreciate the time you're dedicating to bashunit and, of course, we're going to consider the use of tput (or an alternative) now that we're aware of the problem we have and the advantages of using this tool.

But as I mentioned before, we're not comfortable making large changes that have a significant impact on bashunit overall (and this is evident by the number of tests that fail or would need to be changed), so for now, this is the reason why we prefer to set it aside for a more in-depth investigation.

I have created these 2 issues (#246 and #247) to not lose the progress that has emerged from this PR and to know where to continue.

h0adp0re commented 4 months ago

Thanks for the issues, I can do the changes tomorrow.

antonio-gg-dev commented 4 months ago

Thank you very much @h0adp0re for contributing to bashunit!