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

Hey! Spec suite! Leave them streams alone! #175

Merged
merged 2 commits into from
Mar 9, 2015

Conversation

JoshCheek
Copy link
Contributor

Apparently this https://gist.github.com/adamstegman/926858 was to get
RSpec to stfu. But now it calmed down again, so not necessary.
And it affects other things, like swallowing print statements,
causing pry to omit output, and hiding other output that is potentially
legitimate.

However, one nice side effect was that it caught other loudmouths in the dragnet:

  • ActiveRecord::Migration, apparently there is a globaly flag you set to
    tell it to shush.
  • Thor::Shell... I gave up on this one. Had tried passing in shell
    like any sensible interface, but it was blowing up pretty hard. Had
    tried wiping out here https://github.com/erikhuda/thor/blob/53b3fc89ce750258e19c53b1a99b32dd5941451a/lib/thor/shell.rb#L6
    but it's passing around classes instead of instances, so its like you
    have to have layers on layers of fake classes. Tried passing it here
    https://github.com/erikhuda/thor/blob/53b3fc89ce750258e19c53b1a99b32dd5941451a/lib/thor/shell.rb#L53
    but the indirection of who'se inheriting from whom and calling what on
    what and which vars got set by whatever wherever were just too much
    for me at this hour, so fuck it. Stub it.
  • CodeClimate is logging directly to stderr. You can get it to stfu with:
    CodeClimate::TestReporter.configuration.logger = Logger.new('/dev/null')
    Put it before the CodeClimate::TestReporter.start I didn't do it b/c
    it was doing this before (it runs before output streams were silenced)
    so I figured maybe someone wanted it that way on purpose. Plus, you
    probably do want it logging in CI, which you'd probably do by setting an
    env var there, and then silencing it unless you see the var. Since I
    don't have access to your CI, and getting all that config wired up
    right tends to be fragile, I figured it'd be best to leave it.

Went with a matrix theme, b/c the vindictive side of me enjoyed the idea
of Thor's stupid mouth getting melted shut.

Apparently this https://gist.github.com/adamstegman/926858 was to get
RSpec to stfu. But now it calmed down again, so not necessary.
And it affects other things, like swallowing print statements,
causing pry to omit output, and hiding other output that is potentially
legitimate.

However, one nice side effect was that it caught other loudmouths in the dragnet:

* ActiveRecord::Migration, apparently there is a globaly flag you set to
  tell it to shush.
* Thor::Shell... I gave up on this one. Had tried passing in `shell`
  like any sensible interface, but it was blowing up pretty hard. Had
  tried wiping out here https://github.com/erikhuda/thor/blob/53b3fc89ce750258e19c53b1a99b32dd5941451a/lib/thor/shell.rb#L6
  but it's passing around classes instead of instances, so its like you
  have to have layers on layers of fake classes. Tried passing it here
  https://github.com/erikhuda/thor/blob/53b3fc89ce750258e19c53b1a99b32dd5941451a/lib/thor/shell.rb#L53
  but the indirection of who'se inheriting from whom and calling what on
  what and which vars got set by whatever wherever were just too much
  for me at this hour, so fuck it. Stub it.
* CodeClimate is logging directly to stderr. You can get it to stfu with:
  `CodeClimate::TestReporter.configuration.logger = Logger.new('/dev/null')`
  Put it before the `CodeClimate::TestReporter.start` I didn't do it b/c
  it was doing this before (it runs before output streams were silenced)
  so I figured maybe someone wanted it that way on purpose. Plus, you
  probaly do want it logging in CI, which you'd probably do by setting an
  env var there, and then silencing it unless you see the var. Since I
  don't have access to your CI, and getting all that config wired up
  right tends to be fragile, I figured it'd be best to leave it.

Went with a matrix theme, b/c the vindictive side of me enjoyed the idea
of Thor's stupid mouth getting melted shut.
# Apparently Thor's last name is Odinson... who knew?!
# http://marvel.com/universe/Thor_(Thor_Odinson)
# http://www.infinitelooper.com/?v=4D7cPH7DHgA#/170;230
def tell_me_mr_odinson_what_good_is_a_phone_call_if_youre_unable_to_speak?
Copy link
Contributor

Choose a reason for hiding this comment

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

what a weird name!

But, this method should probably be called slience_thor. Or named whatever the actual function is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybeh. But since the function is reveling in melting shut Thor's loud smug mouth, I think it fits. I included a video for context.

Copy link
Contributor

Choose a reason for hiding this comment

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

still hanging on to that method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From context, I assume you want me to change the method name, but that comment seems passive aggressive, so I'm not going to. You're just as capable of changing it as I am, you could have merged it, changed it, pushed it, a month ago, with three minutes worth of effort. I'm not going to sanitize the humour for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, apologies. I didn't mean to come off as rude. I just wasn't sure if you were being serious about the naming. Also, I thought it was poor form to push changes to other people's PRs. If that's how you feel though, I suppose...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I probably predisposed my self to interpret it that way, due to my own emotional immaturity -- primarily a distrust/dislike of authority, which shades my view of anyone who seems to express it. That's my shit, not your fault, no need to apologize. I'll put more emphasis in my brain on remembering everyone's humanity and giving them the benefit of the doubt :)

But yeah, I like this method name. It gives me a sense of serenity. I have no problem with you changing it to better suit your coding standards, but if I have to be the one to do it, it'll feel like an imposition. So, seems best for both of us.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 ❤️ 💯

Amendment to bellycard#175

This line shouldn't have made it in, it was a test to check if this would fix
CodeClimate logging to stderr about how it's not going to do any reporting.
But I meant to omit it b/c IDK how Code Climate works on Travis.
I'm not even really sure this is the best way to do that.
For example, an if statement around the require and start might make more sense
than neutering its logging.

Actually, writing that last sentence just incited me to look at the
[docs](http://docs.travis-ci.com/user/ci-environment/#Environment-variables).
Appears that you could just do this:

```ruby

if ENV['CI'] == 'true'
  require "codeclimate-test-reporter"
  CodeClimate::TestReporter.start
end

```

I'll let y'all do that, though, b/c where you report test coverage from
is a different issue than the test suite swallowing its own output.
@shaqq
Copy link
Contributor

shaqq commented Mar 7, 2015

+1

itsbagpack pushed a commit that referenced this pull request Mar 9, 2015
Hey! Spec suite! Leave them streams alone!
@itsbagpack itsbagpack merged commit 6270216 into bellycard:master Mar 9, 2015
dwebdevcore added a commit to dwebdevcore/napa that referenced this pull request Dec 29, 2022
Amendment to bellycard/napa#175

This line shouldn't have made it in, it was a test to check if this would fix
CodeClimate logging to stderr about how it's not going to do any reporting.
But I meant to omit it b/c IDK how Code Climate works on Travis.
I'm not even really sure this is the best way to do that.
For example, an if statement around the require and start might make more sense
than neutering its logging.

Actually, writing that last sentence just incited me to look at the
[docs](http://docs.travis-ci.com/user/ci-environment/#Environment-variables).
Appears that you could just do this:

```ruby

if ENV['CI'] == 'true'
  require "codeclimate-test-reporter"
  CodeClimate::TestReporter.start
end

```

I'll let y'all do that, though, b/c where you report test coverage from
is a different issue than the test suite swallowing its own output.
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.

4 participants