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

Fix CI detection to avoid unwanted TTY behavior #5804

Merged
merged 9 commits into from Jan 30, 2019

Conversation

@kachkaev
Copy link
Contributor

commented Jan 27, 2019

Fixes #5801

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If not an internal change) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.
kachkaev added 2 commits Jan 27, 2019
@kachkaev

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

What can we do with the code coverage getting a bit lower? I believe it's because we can't mock is-ci without wrapping it into a function, but the function never gets called.

@SimenB

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

jest.mock('is-ci', () => true)?

@kachkaev

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

is-ci returns a value, not a function, which makes its mocking problematic. Besides, all mockable modules have to be 'proxied' via third-party.js – otherwise, they can't be mocked in prod tests.

// We cannot use `jest.setMock("get-stream", impl)` here, because in the
// production build everything is bundled into one file so there is no
// "get-stream" module to mock.

@SimenB

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Ah, didn't know about the issue when bundling

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Jan 27, 2019

Maybe off-topic but anyway … this seems to be the entire is-ci module (https://github.com/watson/is-ci/blob/8b88365d804abc1905ca93f312e99cf95b48fc8a/index.js):

'use strict'

module.exports = require('ci-info').isCI

So … is it better to simply depend on ci-info?

@SimenB

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Won't make a difference, require('ci-info') will still give you an object (instead of a boolean - none are functions)

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Jan 27, 2019

Yeah, I didn’t mean it would help the tests. I guess I was mostly wondering if anybody else was surprised by the seemingly pointless wrapper package :)

@ikatyang
Copy link
Member

left a comment

Could you add a changelog item in CHANGELOG.unrelease.md since the change is visible to users?

src/cli/util.js Outdated Show resolved Hide resolved

@ikatyang ikatyang added this to the 1.16.2 milestone Jan 28, 2019

src/utils/is-tty.js Outdated Show resolved Hide resolved

@kachkaev kachkaev force-pushed the kachkaev:fix-ci-detection branch from f235d59 to 10ef661 Jan 28, 2019

@DSchau

This comment has been minimized.

Copy link

commented Jan 29, 2019

Thanks for the quick turnaround on this everyone! Just checking to see if this is scheduled for a release soon--we'd love this functionality!

Thanks again!

@ikatyang ikatyang merged commit 36aeb4c into prettier:master Jan 30, 2019

6 of 8 checks passed

Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 3 redirect rules processed
Details
codecov/patch 88.88% of diff hit (target 80%)
Details
codecov/project 94.4% (+<.01%) compared to 7faa260
Details
deploy/netlify Deploy preview ready!
Details
prettier.prettier Build #20190128.6 succeeded
Details
@ikatyang

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@kachkaev Thanks!

@DSchau Today or tomorrow if everything goes well.

@@ -74,6 +76,9 @@ function runPrettier(dir, args, options) {
jest.spyOn(require(thirdParty), "getStream").mockImplementation(() => ({
then: handler => handler(options.input || "")
}));
jest
.spyOn(require(thirdParty), "isCI")
.mockImplementation(() => process.env.CI);

This comment has been minimized.

Copy link
@j-f1

j-f1 Jan 31, 2019

Member

Should this be mocked to a specific value so results are consistent?

This comment has been minimized.

Copy link
@kachkaev

kachkaev Jan 31, 2019

Author Contributor

We want to test both CI and non-CI modes and I’m setting env: { CI: true } in a couple of integration tests to make sure the behavior changes.

@lock lock bot locked as resolved and limited conversation to collaborators May 1, 2019

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