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

formula: set TERM to dumb during test #982

Merged
merged 1 commit into from Sep 18, 2016

Conversation

Projects
None yet
6 participants
@zmwangx
Copy link
Contributor

zmwangx commented Sep 17, 2016

  • 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?

Software that tries to print different things (e.g. ANSI color) based on different termcaps often relies on the TERM environment variable, and could fail without it. This results in confusing test issues where certain tests can be successfully run by users testing locally with TERM
set, but choke up on our CI.

Always setting TERM to dumb leads to better consistency between local tests and CI tests, and saves much probing in certain cases.


Some examples:

  • aalib: Existing test fails on the CI without TERM — I was investigating the failure just now;

  • hopenpgp-tools: I was confused by this just a short while ago;

  • In core:

    $ grep -rI 'ENV\["TERM"\]'
    Formula/asciiquarium.rb:    ENV["TERM"] = "xterm"
    Formula/browser.rb:    ENV["TERM"] = "xterm"
    Formula/bvi.rb:    ENV["TERM"] = "xterm"
    Formula/clipsafe.rb:    ENV["TERM"] = "dumb"
    Formula/diff-so-fancy.rb:    ENV["TERM"] = "xterm"
    Formula/dshb.rb:    ENV["TERM"] = "xterm"
    Formula/ee.rb:    ENV["TERM"] = "xterm"
    Formula/eg.rb:    ENV["TERM"] = "xterm"
    Formula/git-test.rb:    ENV["TERM"] = "xterm"
    Formula/hopenpgp-tools.rb:    ENV["TERM"] = "dumb"
    Formula/htop-osx.rb:    ENV["TERM"] = "xterm"
    Formula/htop.rb:    ENV["TERM"] = "xterm"
    Formula/jp2a.rb:    ENV["TERM"] = "xterm-256color"
    Formula/le.rb:    ENV["TERM"] = "xterm"
    Formula/mobile-shell.rb:    ENV["TERM"] = "xterm"
    Formula/multitail.rb:    ENV["TERM"] = "xterm"
    Formula/ne.rb:    ENV["TERM"] = "xterm"
    Formula/newt.rb:    ENV["TERM"] = "xterm"
    Formula/pwntools.rb:    ENV["TERM"] = "xterm"
    Formula/slrn.rb:    ENV["TERM"] = "xterm"
    Formula/sngrep.rb:    ENV["TERM"] = "xterm"
    Formula/tnote.rb:    ENV["TERM"] = "xterm"
    Formula/watch.rb:    ENV["TERM"] = "xterm"
    Formula/weechat.rb:    ENV["TERM"] = "xterm"
    

    So many formulae require a workaround that could be easily resolved in general. Setting TERM to xterm is also "wrong" because on the CI the logging facilities definitely don't have xterm capabilities. If a formula specifically requires xterm in order to work, it can still be overridden in the test block as before.

formula: set TERM to dumb during test
Software that tries to print different things (e.g. ANSI color) based on
different termcaps often relies on the TERM environment variable, and
could fail without it. This results in confusing test issues where
certain tests can be successfully run by users testing locally with TERM
set, but choke up on our CI.

Always setting TERM to dumb leads to better consistency between local
tests and CI tests, and saves much probing in certain cases.
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 17, 2016

Another one I see like this is ENV["LC_ALL"] = "en_US.UTF-8"

@zmwangx

This comment has been minimized.

Copy link
Contributor Author

zmwangx commented Sep 17, 2016

$ grep -rI 'ENV\["LC_ALL"\]'
Formula/alot.rb:      ENV["LC_ALL"] = "en_US.UTF-8"
Formula/asciinema.rb:    ENV["LC_ALL"] = "en_US.UTF-8"
Formula/cmake.rb:      ENV["LC_ALL"] = "en_US.UTF-8"
Formula/khal.rb:    ENV["LC_ALL"] = "en_US.UTF-8"
Formula/lensfun.rb:    ENV["LC_ALL"] = "en_US.UTF-8"
Formula/mpv.rb:    ENV["LC_ALL"] = "C"
Formula/percona-xtrabackup.rb:      ENV["LC_ALL"] = "en_US.UTF-8"
Formula/pixz.rb:    ENV["LC_ALL"] = "en_US.UTF-8"
Formula/twtxt.rb:    ENV["LC_ALL"] = "en_US.UTF-8"
Formula/vdirsyncer.rb:    inreplace prefix/"vdirsyncer.plist", "@@LOCALE@@", ENV["LC_ALL"] || ENV["LANG"] || "en_US.UTF-8"
Formula/vdirsyncer.rb:    ENV["LC_ALL"] = "en_US.UTF-8"
$ grep -rI 'ENV\["LANG"\]'
Formula/khal.rb:    ENV["LANG"] = "en_US.UTF-8"
Formula/lesstif.rb:    ENV["LANG"] = "C"
Formula/mitmproxy.rb:    ENV["LANG"] = "en_US.UTF-8"
Formula/twtxt.rb:    ENV["LANG"] = "en_US.UTF-8"
Formula/vdirsyncer.rb:    inreplace prefix/"vdirsyncer.plist", "@@LOCALE@@", ENV["LC_ALL"] || ENV["LANG"] || "en_US.UTF-8"

Reasonable.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 17, 2016

I guess the argument against globally setting the LC_ALL is that locally people probably don't have that set either.

@zmwangx

This comment has been minimized.

Copy link
Contributor Author

zmwangx commented Sep 17, 2016

"Reasonable" expands to "your suggestion is reasonable."

@zmwangx

This comment has been minimized.

Copy link
Contributor Author

zmwangx commented Sep 17, 2016

Oh I see what you're saying.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 17, 2016

probably best to stick to just TERM for now I guess

@zmwangx

This comment has been minimized.

Copy link
Contributor Author

zmwangx commented Sep 17, 2016

That won't be a problem once we have #932 (although I haven't thought through all the ramifications it might have).

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

👍 as long as you've tested it on a bunch of formulae that set multiple different values and they still all work.

@zmwangx

This comment has been minimized.

Copy link
Contributor Author

zmwangx commented Sep 17, 2016

@MikeMcQuaid I did test on a bunch of listed formulae locally (not all) and I don't see any problem moving from xterm to dumb. CI might disagree but I highly doubt it.

This change isn't noticeable to users so I think we can merge first and test the affected formulae later. Even if a formula or two do require xterm, as I said above we can still override in the test block.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 17, 2016

@zmwangx Cool :shipit:

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

Agreed. This sounds like a good approach. And since most users won't see it, seems reasonable to merge first and test primarily through the CI.

@zmwangx zmwangx merged commit 7926114 into Homebrew:master Sep 18, 2016

4 checks passed

codecov/patch Coverage not affected when comparing a54c968...f8d7745
Details
codecov/project Absolute coverage decreased by -0.84% but relative coverage increased by +16.28% compared to a54c968
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@zmwangx zmwangx deleted the zmwangx:dumb-term-in-test branch Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

diff-so-fancy: fix test
diff-so-fancy needs some actual diff as input, or it will just hang
there waiting for input from stdin.

Also, do not manually set TERM in test, which should no longer be
necessary since Homebrew/brew#982.

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

dshb: do not manually set TERM in test
No longer necessary since Homebrew/brew#982.

In fact, dshb being a TUI application does require xterm or at least
some basic termcap to function (show anything meaningful), but for this
test a dumb term is enough.

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

diff-so-fancy: fix test
diff-so-fancy needs some actual diff as input, or it will just hang
there waiting for input from stdin.

Also, do not manually set TERM in test, which should no longer be
necessary since Homebrew/brew#982.

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to zmwangx/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

BrewTestBot added a commit to BrewTestBot/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

zmwangx added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016

weechat: do not manually set TERM in test
No longer necessary since Homebrew/brew#982.

Closes #4941.

Signed-off-by: Zhiming Wang <zmwangx@gmail.com>
@zmwangx

This comment has been minimized.

Copy link
Contributor Author

zmwangx commented Sep 18, 2016

To wrap this up: I've removed ENV["TERM"] from tests where possible, and that leaves us with seven formulae that require xterm:

$ grep -rIl 'ENV\["TERM"\]'
Formula/asciiquarium.rb
Formula/ee.rb
Formula/eg.rb
Formula/git-test.rb
Formula/ne.rb
Formula/newt.rb
Formula/slrn.rb

which is a still a good thing because it's now possible to determine locally that those settings are required; before this PR you can remove the ENV["TERM"] line from the tests, the tests would still work locally in xterm-capable terminals, but they'll fail (sometimes mysteriously) on the CI.

Note: To answer the possible question of why we don't set TERM to xterm universally: masquerading as xterm while not having the corresponding termcaps is a dangerous thing to do in general.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.