Skip to content

Fix CI detection to avoid unwanted TTY behavior#5804

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

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

Conversation

@kachkaev
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

SimenB commented Jan 27, 2019

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

@kachkaev
Copy link
Copy Markdown
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
Copy Markdown
Contributor

SimenB commented Jan 27, 2019

Ah, didn't know about the issue when bundling

@lydell
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@ikatyang ikatyang added this to the 1.16.2 milestone Jan 28, 2019
lydell
lydell reviewed Jan 28, 2019
View reviewed changes
@kachkaev kachkaev mentioned this pull request Jan 28, 2019
Closed
@DSchau
Copy link
Copy Markdown

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
Copy Markdown
Member

ikatyang commented Jan 30, 2019

@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
}));
jest
.spyOn(require(thirdParty), "isCI")
.mockImplementation(() => process.env.CI);
Copy link
Copy Markdown
Member

@j-f1 j-f1 Jan 31, 2019

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

@kachkaev kachkaev Jan 31, 2019

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