Fix CI detection to avoid unwanted TTY behavior

[kachkaev]

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]

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]

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

[kachkaev]

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.

prettier/tests_integration/runPrettier.js

Lines 71 to 73 in 18b03a3

	// 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]

Ah, didn't know about the issue when bundling

[lydell]

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]

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

[lydell]

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]

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

[DSchau]

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]

@kachkaev Thanks!

@DSchau Today or tomorrow if everything goes well.

[j-f1]

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

[kachkaev]

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.