Skip to content

Add a $command variable, and colorize summary#444

Closed
msterin wants to merge 1 commit intobats-core:masterfrom
kontainapp:msterin/color-and-command
Closed

Add a $command variable, and colorize summary#444
msterin wants to merge 1 commit intobats-core:masterfrom
kontainapp:msterin/color-and-command

Conversation

@msterin
Copy link
Copy Markdown

@msterin msterin commented May 11, 2021

Fixes #163

Motivation:

  • See Add a $command variable that captures arguments passed to run #163 for $command motivation. I second it and also it's helpful when the command passed to run contains multiple shell vars, so $command shows the final.
  • Our test pass is fairly long and it's super convenient to have colorized summary (pass/fail) at the end of manual runs

We use bats with these fixes for a long time now, time for a PR.

@msterin msterin requested a review from a team as a code owner May 11, 2021 00:25
martin-schulze-vireso
martin-schulze-vireso requested changes May 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@martin-schulze-vireso martin-schulze-vireso left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. Could you add tests for these new features.

Comment thread lib/bats-core/test_functions.bash Outdated
Comment thread libexec/bats-core/bats-format-pretty Outdated
@msterin
Copy link
Copy Markdown
Author

msterin commented May 11, 2021 •
edited
Loading

@martin-schulze-vireso - thanks for the review !

I've just pushed the requested changes.

If there is a better way of color test than to grab an escape sequences from output, I'd be happy to use :-).

Thanks

@msterin msterin force-pushed the msterin/color-and-command branch 4 times, most recently from 7623a53 to 8ac2b7d Compare May 12, 2021 00:53
Add a $command variable, and colorize summary
565f286 
Fixes bats-core#163

Motivation:
* See bats-core#163 for $command motivation. I second it and also it's helpful when the command passed to `run` contains multiple shell vars, so $command shows the final.
* Our test pass is 400+ tests and it's super convenient to have colorized summary (pass/fail) at the end of manual runs

We use bats with these fixes for long time now, time for a PR.

- [x] I have reviewed the [Contributor Guidelines][contributor].
- [x] I have reviewed the [Code of Conduct][coc] and agree to abide by it

[contributor]: https://github.com/bats-core/bats-core/blob/master/docs/CONTRIBUTING.md
[coc]:         https://github.com/bats-core/bats-core/blob/master/docs/CODE_OF_CONDUCT.md
@msterin msterin force-pushed the msterin/color-and-command branch from 8ac2b7d to 565f286 Compare May 12, 2021 04:33
@msterin msterin mentioned this pull request May 25, 2021
Open
@martin-schulze-vireso
Copy link
Copy Markdown
Member

martin-schulze-vireso commented Jun 6, 2021

I am still mulling over whether it is okay to use $command. It sure is in line with the other variables ($output, $lines, $status). However, the new $command might break existing code of the form:

command="some user defined value"
run some command # overrides $command's value
[ "$command" = "some user defined value"] # expectation is for the old, user defined value

@bats-core/bats-core What do you think? Should we proceed with this and place a big fat warning in the release notes? Should we use a name that is less likely to collide, like bats_run_command?

@msterin
Copy link
Copy Markdown
Author

msterin commented Jun 9, 2021

Good point and I am happy to change to whatever there is an agreement about, e.g.

  • command_line (reasonably aligned with other vars and IMO very unlikely to generate a conflict - google search for "command_line" AND "bats" AND site:github.com brings nothing)
    or
  • bats_run_command

Just let me know what's the maintainers' preference.

@rockandska
Copy link
Copy Markdown
Contributor

rockandska commented Jun 9, 2021

@bats-core/bats-core What do you think? Should we proceed with this and place a big fat warning in the release notes? Should we use a name that is less likely to collide, like bats_run_command?

Not directly related to this PR (sorry to pollute @msterin), but in a general way, I think that all internal variables / functions used by bats and exposed in @test should have a prefix (bats_) to avoid collision with tested code at maximum.
As consequences, external libraries / users tests will break and should be changed in a major release.

@martin-schulze-vireso
Copy link
Copy Markdown
Member

martin-schulze-vireso commented Jul 25, 2021

This might become obsolete through #467 which (currently) adds BATS_TRACE_COMMAND.

@martin-schulze-vireso martin-schulze-vireso mentioned this pull request Jul 25, 2021
Closed
@NorseGaud
Copy link
Copy Markdown
Contributor

NorseGaud commented Jul 25, 2021

@martin-schulze-vireso , they look compatible. I added the code manually to test it out and it seems to work.

@NorseGaud
Copy link
Copy Markdown
Contributor

NorseGaud commented Jul 27, 2021

@msterin , you going to continue with getting this ready? If not, I'd like to take over and handle it.

@msterin
Copy link
Copy Markdown
Author

msterin commented Jul 27, 2021

@msterin , you going to continue with getting this ready? If not, I'd like to take over and handle it.

@NorseGaud Please, do take over - I just started a new job and will have very limited cycles for a while. Appreciate it.

@NorseGaud
Copy link
Copy Markdown
Contributor

NorseGaud commented Jul 27, 2021

@msterin , you going to continue with getting this ready? If not, I'd like to take over and handle it.

@NorseGaud Please, do take over - I just started a new job and will have very limited cycles for a while. Appreciate it.

Best of luck with the new job! That's exciting!

@NorseGaud
Copy link
Copy Markdown
Contributor

NorseGaud commented Jul 28, 2021

@martin-schulze-vireso I'm going to figure out how to do this in my existing --trace branch. You can close this PR.

martin-schulze-vireso pushed a commit to NorseGaud/bats-core that referenced this pull request Aug 17, 2021
@NorseGaud @martin-schulze-vireso
pretty: Colorize summary (refs bats-core#163, bats-core#444)
945dfed
martin-schulze-vireso pushed a commit to NorseGaud/bats-core that referenced this pull request Aug 20, 2021
@NorseGaud @martin-schulze-vireso
pretty: Colorize summary (refs bats-core#163, bats-core#444)
0dd8f31
@rockandska rockandska mentioned this pull request Nov 30, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-schulze-vireso martin-schulze-vireso martin-schulze-vireso requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a $command variable that captures arguments passed to run

4 participants

@msterin @martin-schulze-vireso @rockandska @NorseGaud