Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase contrast of test results #245

Merged
merged 6 commits into from
Mar 2, 2024

Conversation

h0adp0re
Copy link
Contributor

@h0adp0re h0adp0re commented Feb 26, 2024

📚 Description

Improve test report contrast.

Before

Screenshot 2024-02-26 at 19 17 04

After

Screenshot 2024-02-26 at 19 37 38

🔖 Changes

  • Improve the final test result state contrast.
  • Add whitespace to the final test result state.
  • Fix "filer" -> "filter" typo in the help command.
  • Simplify .editorconfig to specify as little as possible, making the file more readable.
    The root cause of this change was neovim not respecting the defined indents for .sh files.

✅ To-do list

  • I updated the CHANGELOG.md to reflect the new feature or fix
  • I updated the documentation to reflect the changes (N/A?)

src/assert.sh Outdated Show resolved Hide resolved
@h0adp0re
Copy link
Contributor Author

h0adp0re commented Feb 26, 2024

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
Copy link
Member

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

@h0adp0re
Copy link
Contributor Author

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

@h0adp0re h0adp0re force-pushed the fix/output-colors branch 3 times, most recently from 2633819 to 4fe733e Compare February 27, 2024 00:06
@h0adp0re
Copy link
Contributor Author

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
Copy link
Member

More broken tests on the CI @h0adp0re 🙏🏻

@h0adp0re
Copy link
Contributor Author

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

@h0adp0re
Copy link
Contributor Author

h0adp0re commented Feb 27, 2024

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@h0adp0re h0adp0re changed the title Refactor output colors to use tput, increase contrast of test report Increase contrast of test results Mar 1, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? It does make the test more readable, but the use of variables may defeat the purpose of the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that using app global variables removes part of the test's purpose, but it's as simple as redeclaring them locally (with different names) within the test and not using the application's global ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's all done now.

@antonio-gg-dev
Copy link
Member

Thank you very much @h0adp0re for contributing to bashunit!

@antonio-gg-dev antonio-gg-dev merged commit 1507eb6 into TypedDevs:main Mar 2, 2024
7 checks passed
@h0adp0re h0adp0re deleted the fix/output-colors branch March 2, 2024 18:28
antonio-gg-dev added a commit that referenced this pull request Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants