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.

Sign up for GitHub

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

Jump to bottom

Fix CI detection to avoid unwanted TTY behavior #5804

Merged
merged 9 commits into from
Jan 30, 2019
Merged

Fix CI detection to avoid unwanted TTY behavior #5804

merged 9 commits into from
Jan 30, 2019

Conversation

kachkaev
Copy link
Member

@kachkaev kachkaev commented Jan 27, 2019 •
edited
Loading

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
Copy link
Member Author

kachkaev commented Jan 27, 2019 •
edited
Loading

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
Copy link
Contributor

SimenB commented Jan 27, 2019

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

@kachkaev
Copy link
Member Author

kachkaev commented Jan 27, 2019 •
edited
Loading

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
Copy link
Contributor

SimenB commented Jan 27, 2019

Ah, didn't know about the issue when bundling

@lydell
Copy link
Member

lydell 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
Copy link
Contributor

SimenB commented Jan 27, 2019 •
edited
Loading

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

@lydell
Copy link
Member

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

@ikatyang ikatyang left a comment

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?

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

DSchau 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
@ikatyang
Copy link
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
tests_integration/runPrettier.js
@@ -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);
Copy link
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?

Copy link
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

@j-f1 j-f1 j-f1 left review comments

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