Convert Tty test to spec. #2040

reitermarkus · 2017-02-16T21:43:28Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

alyssais · 2017-02-17T18:23:08Z

Library/Homebrew/test/utils/tty_spec.rb

+require "utils"
+
+describe Tty do
+  specify "::strip_ansi" do


Can we use spec names that describe the behaviour rather than just naming a method where possible? These cases seem pretty straightforward, e.g. "::strip_ansi removes ANSI escapes from a String".

#2044 (comment) also applies here.

alyssais · 2017-02-17T18:23:34Z

Library/Homebrew/test/utils/tty_spec.rb

+
+  specify "::width" do
+    expect(described_class.width).to be_kind_of(Integer)
+    expect(described_class.width).to be >= 0


I think these should be different tests, like before.

alyssais · 2017-02-17T18:24:38Z

Library/Homebrew/test/utils/tty_spec.rb

@@ -0,0 +1,54 @@
+require "utils"
+
+describe Tty do


#2044 (comment) also applies here.

alyssais · 2017-02-17T18:26:12Z

Library/Homebrew/test/utils/tty_spec.rb

+  context "$stdout is not a TTY" do
+    it "returns an empty string for all colors" do
+      allow($stdout).to receive(:tty?).and_return(false)
+      expect(described_class.to_s).to eq("")


I think we should use be_empty here.

Not sure if that's better here, since eq("") tests if it is an empty string, not just if it's empty.

It seems pretty unlikely to me that #to_s would return, e.g. an Array, but it's up to you.

I am aware that this is pretty unlikely, but I like getting two tests for the price of one. 😄

alyssais · 2017-02-17T18:26:40Z

Library/Homebrew/test/utils/tty_spec.rb

+
+  context "$stdout is not a TTY" do
+    it "returns an empty string for all colors" do
+      allow($stdout).to receive(:tty?).and_return(false)


This should be in a before block since it's doing what's described in the context.

alyssais · 2017-02-17T18:26:57Z

Library/Homebrew/test/utils/tty_spec.rb

+
+  context "$stdout is not a TTY" do
+    it "returns an empty string for all colors" do
+      allow($stdout).to receive(:tty?).and_return(true)


This should be in a before block since it's doing what's described in the context.

alyssais · 2017-02-17T18:27:09Z

Library/Homebrew/test/utils/tty_spec.rb

+  context "$stdout is not a TTY" do
+    it "returns an empty string for all colors" do
+      allow($stdout).to receive(:tty?).and_return(true)
+      expect(described_class.to_s).to eq("")


I think we should use be_empty here.

alyssais · 2017-02-17T19:31:49Z

Library/Homebrew/test/utils/tty_spec.rb

+require "utils"
+
+describe Tty do
+  subject { described_class }


I don't think this line is necessary. Haven't checked, but I think RSpec automatically sets subject like this when described_class is a module.

reitermarkus requested review from MikeMcQuaid and alyssais February 16, 2017 21:43

alyssais suggested changes Feb 17, 2017

View reviewed changes

reitermarkus force-pushed the spec-utils_tty branch from a753d99 to ae6618d Compare February 17, 2017 19:04

alyssais reviewed Feb 17, 2017

View reviewed changes

Convert Tty test to spec.

8736b6c

reitermarkus force-pushed the spec-utils_tty branch from ae6618d to 8736b6c Compare February 17, 2017 23:01

alyssais approved these changes Feb 17, 2017

View reviewed changes

reitermarkus merged commit 61a41d0 into Homebrew:master Feb 18, 2017

reitermarkus deleted the spec-utils_tty branch February 18, 2017 14:06

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Tty test to spec. #2040

Convert Tty test to spec. #2040

reitermarkus commented Feb 16, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

reitermarkus Feb 17, 2017

alyssais Feb 17, 2017

reitermarkus Feb 17, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

alyssais Feb 17, 2017

reitermarkus Feb 17, 2017

Convert Tty test to spec. #2040

Convert Tty test to spec. #2040

Conversation

reitermarkus commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment