Skip to content

Stabilize colors #1524

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

Merged
merged 4 commits into from
Sep 24, 2017
Merged

Stabilize colors #1524

merged 4 commits into from
Sep 24, 2017

Conversation

jugglinmike
Copy link
Contributor

Stabilize colors

Prior to this commit, the library and its tests relied on the same code
to add color to terminal output. This prevented the tests from
recognizing changes to that code.

Such a change was introduced in a recent release of the chalk module
[1]: it was updated to disable text coloring when run without a TTY.
This is the case in the test environment environment provided by the
tap module (which Ava uses for its tests).

This fix disabled color output in the tests, but it caused no failures
because both the expected values and the actual values were effected.
However, this situation amounted to a decrease in test coverage because
the expected color of the terminal output could no longer be verified.

Update the tests environment to simulate a TTY so that Ava continues to
produce colored text despite being run in a sub-process provided by
tap. Update the test logic to reduce dependency on Ava internals in
order to avoid similar silent regressions in the future.

[1] chalk/chalk@0827d3b

This issue was a surprise for me after first submitting gh-1508 because I had been using npm version 3. That version does not recognize the package_lock.json file, so I had been vetting my changes with newer versions of the module dependencies than are available in the continuous integration environment (this is why the tests pass for the build that sets FRESH_DEPS to true).

Prior to this commit, the library and its tests relied on the same code
to add color to terminal output. This prevented the tests from
recognizing changes to that code.

Such a change was introduced in a recent release of the `chalk` module
[1]: it was updated to disable text coloring when run without a TTY.
This is the case in the test environment environment provided by the
`tap` module (which Ava uses for its tests).

This fix disabled color output in the tests, but it caused no failures
because both the expected values and the actual values were effected.
However, this situation amounted to a decrease in test coverage because
the expected color of the terminal output could no longer be verified.

Update the tests environment to simulate a TTY so that Ava continues to
produce colored text despite being run in a sub-process provided by
`tap`. Update the test logic to reduce dependency on Ava internals in
order to avoid similar silent regressions in the future.

[1] chalk/chalk@0827d3b
@novemberborn
Copy link
Member

This issue was a surprise for me after first submitting gh-1508 because I had been using npm version 3. That version does not recognize the package_lock.json file, so I had been vetting my changes with newer versions of the module dependencies than are available in the continuous integration environment (this is why the tests pass for the build that sets FRESH_DEPS to true).

This is odd. The package-lock.json can help ensure everybody has the same dependencies when developing, and stop us from accidentally using a dependency's API that came with a newer version of that dependency than is in our SemVer range. Sounds like there is an unexpected breaking change in perhaps chalk? If we update our version of chalk would that help? How does #1401 impact this problem?

I'm perpetually confused by color management in AVA, and this PR seems appropriate despite my questions in the previous paragraph. Still, I'd like to understand it all a bit better before hitting the merge button.

(And, as it stands, there are some linting errors.)

@jugglinmike
Copy link
Contributor Author

Sounds like there is an unexpected breaking change in perhaps chalk? If we
update our version of chalk would that help? How does #1401 impact this
problem?

I think I'd call it a bug fix because as I understand it, it disables colors
when a TTY is not present, which generally seems expected. I don't believe that
the referenced issue about the --color flag is directly related because the
test process is being created by tap, not Ava itself.

With the linting problem out of the way, I can see some Windows-specific test
failures. It looks like there are some complexities in Chalk that need to be
re-created in the new test helper. I submitted another "fixup" commit to
implement that. In some ways, it's unfortunate for the test logic to grow in
complexity like this. In other ways, this is functionality that consumers
depend on, so it's probably for the best that it is explicitly guaranteed by
this project's tests.

@novemberborn
Copy link
Member

Makes sense, thanks @jugglinmike!

@novemberborn novemberborn merged commit b888419 into avajs:master Sep 24, 2017
@jugglinmike
Copy link
Contributor Author

My pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants