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

Colorize summarized RSpec results. #787

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

joshuapinter
Copy link
Contributor

@joshuapinter joshuapinter commented Nov 8, 2020

RSpec colorizes its results based on if there are any failures (red), pending (orange) or none of the above (green).

The method used to summarize the results from the various threads, summarize_results was not doing this colorization.

This commit applies the same colorization that RSpec does, utilizing RSpec::Core helpers to do so.

Before

Screenshot 2020-11-08 at 10 27 05

Screenshot 2020-11-08 at 10 17 20

Screenshot 2020-11-08 at 10 14 45

After

Screenshot 2020-11-08 at 10 10 25

Screenshot 2020-11-08 at 10 07 12

Screenshot 2020-11-08 at 10 12 26

Thank you for your contribution!

Checklist

  • Feature branch is up-to-date with master (if not - rebase it).
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • Update Readme.md when cli options are changed

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

also: sorry for the long silence, this has been buried in my inbox :D

# Summarize results from threads and colorize results based on failure and pending counts.
#
def summarize_results(results)
sums = sum_up_results(results)
Copy link
Owner

Choose a reason for hiding this comment

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

can move this under the return

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

can you add a test for the coloration happening ?

@joshuapinter
Copy link
Contributor Author

All good! I honestly forgot about it myself. :)

I took your suggested change and removed the rspec dependency. Added an entry to CHANGELOG as well.

Will just add some tests and then pass it back to you.

@joshuapinter
Copy link
Contributor Author

Quick Q: Are these CI tests supposed to be failing. Seem unrelated to my changes...

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

trying to fix CI here #788

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

please rebase, at least ubuntu should be fixed

@joshuapinter
Copy link
Contributor Author

Will squash and rebase...

@joshuapinter
Copy link
Contributor Author

Odd, it's not letting me rebase and force push. I think because it includes your workflow commit:

 ! [remote rejected] colorize_rspec_results -> colorize_rspec_results (refusing to allow an OAuth App to create or update workflow `.github/workflows/test.yml` without `workflow` scope)
error: failed to push some refs to 'https://github.com/cntral/parallel_tests.git'

Super strange. I'm going open a new PR and see if that helps.

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

might be some security foobar having to do with the PR being older than the GA addition :(

... can you make a new branch and then pick this commit on top ?

@joshuapinter
Copy link
Contributor Author

Damn, I can't even update my own fork's master from your master.

! [remote rejected] master -> master (refusing to allow an OAuth App to create or update workflow `.github/workflows/test.yml` without `workflow` scope)

That's restrictive.

I might be forced to create a new fork entirely. Super strange.

@joshuapinter
Copy link
Contributor Author

Okay, the only way I could update my fork's master was to create a PR from your master to my master in Github's UI.

Then I was able to rebase my branch and force push. It's still showing your commit in this branch for some reason but 🤷‍♂️ .

Take a look and let me know if you want or need anything else.

Thanks.

@joshuapinter
Copy link
Contributor Author

Kk, nevermind. I was able to fix the extra commit with another rebase. Strange times. Over to you...

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

       expected: "\e[31m1 example, 1 failure, 1 pending\e[0m"
            got: "1 example, 1 failure, 1 pending"

needs an update ?

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

... could strip extra characters in all other tests with a regex

@grosser
Copy link
Owner

grosser commented Nov 22, 2020

only 1 commit here now :(

@joshuapinter
Copy link
Contributor Author

Strange. Okay, fixed that and re-pushed.

@joshuapinter
Copy link
Contributor Author

Tests still failing.... All tests pass on macOS. Does Ubuntu somehow deal with these differently?

@grosser
Copy link
Owner

grosser commented Nov 23, 2020

might handle tty differently, best to just stub tty? to be true ... or make it accept different results based on if $stdout.tty? in the test

RSpec colorizes its results based on if there are any failures (red), pending (yellow) or none of the above (green).

The method used to summarize the results from the various threads, `summarize_results` was not doing this colorization.

This commit applies the same colorization that RSpec does but does not depend on RSpec::Core for colorization. It utilizes red for failure, yellow for pending and green for success.

- Entry in `CHANGELOG.md` added.
- Specs added for all major scenarios:
  - No failures or pending.
  - Just pending.
  - Failures and pending.
  - Just failures.

Co-authored-by: Michael Grosser <michael@grosser.it>
@joshuapinter
Copy link
Contributor Author

might handle tty differently, best to just stub tty? to be true ... or make it accept different results based on if $stdout.tty? in the test

Good call. 👍

@grosser grosser merged commit f646fd2 into grosser:master Nov 23, 2020
@joshuapinter joshuapinter deleted the colorize_rspec_results branch November 23, 2020 02:07
@grosser
Copy link
Owner

grosser commented Nov 23, 2020

thx, 3.4.0 🎉

@joshuapinter
Copy link
Contributor Author

Thanks for the help and shipping a new version so quickly! 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants