Skip to content

Fix CI detection to avoid unwanted TTY behavior - #5804

Merged
ikatyang merged 9 commits into
prettier:masterfrom
kachkaev:fix-ci-detection
Jan 30, 2019
Merged

Fix CI detection to avoid unwanted TTY behavior#5804
ikatyang merged 9 commits into
prettier:masterfrom
kachkaev:fix-ci-detection

Conversation

@kachkaev

@kachkaev kachkaev commented Jan 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 kachkaev mentioned this pull request Jan 27, 2019
Closed
@kachkaev

kachkaev commented Jan 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

SimenB commented Jan 27, 2019

Copy link
Copy Markdown
Contributor

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

@kachkaev

kachkaev commented Jan 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

SimenB commented Jan 27, 2019

Copy link
Copy Markdown
Contributor

Ah, didn't know about the issue when bundling

@lydell

lydell commented Jan 27, 2019

Copy link
Copy Markdown
Member

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

SimenB commented Jan 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@lydell

lydell commented Jan 27, 2019

Copy link
Copy Markdown
Member

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
ikatyang reviewed Jan 28, 2019
View reviewed changes

@ikatyang ikatyang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread src/cli/util.js Outdated
@ikatyang ikatyang added this to the 1.16.2 milestone Jan 28, 2019
lydell
lydell reviewed Jan 28, 2019
View reviewed changes
Comment thread src/utils/is-tty.js Outdated
@kachkaev kachkaev mentioned this pull request Jan 28, 2019
Closed
@DSchau

DSchau commented Jan 29, 2019

Copy link
Copy Markdown

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
@ikatyang

ikatyang commented Jan 30, 2019

Copy link
Copy Markdown
Member

@kachkaev Thanks!

@DSchau Today or tomorrow if everything goes well.

@automatops automatops mentioned this pull request Jan 30, 2019
Merged
1 task
@ghost ghost mentioned this pull request Jan 30, 2019
Merged
1 task
@xcomponentadmin xcomponentadmin mentioned this pull request Jan 30, 2019
Merged
j-f1
j-f1 reviewed Jan 31, 2019
View reviewed changes
Comment thread tests_integration/runPrettier.js
}));
jest
.spyOn(require(thirdParty), "isCI")
.mockImplementation(() => process.env.CI);

@j-f1 j-f1 Jan 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@kachkaev kachkaev Jan 31, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label May 1, 2019
@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.

Reviewers

@lydell lydell lydell left review comments
@j-f1 j-f1 j-f1 left review comments
@ikatyang ikatyang ikatyang approved these changes

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

1.16.2

Development

Successfully merging this pull request may close these issues.

list-different (and check) log every file in CI, instead of just changed files

6 participants

@kachkaev @SimenB @lydell @DSchau @ikatyang @j-f1