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.

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

feat: Add support for env.NYC_CONFIG_OVERRIDE #1077

Merged
merged 2 commits into from Apr 24, 2019

Conversation

isaacs
Copy link
Collaborator

@isaacs isaacs commented Apr 19, 2019

This allows a process to spawn a child process with an environment
variable NYC_CONFIG_OVERRIDE. This is a JSON string which overrides any
values in the NYC_CONFIG env. It is not deleted in the child process,
so will be contagious unless unset, and it is the responsibility of the
caller to manage.

The first intended use case of this feature is to allow node-tap to map
tests to the specific portion of a system under test that they cover.
In this way, unit tests can be more focused, and only re-run when their
specific unit has changed.

There are, of course, many other uses that this could be put to, and it
did not seem appropriate to add a special hook just for overriding
the include list.

This allows a process to spawn a child process with an environment
variable NYC_CONFIG_OVERRIDE.  This is a JSON string which overrides any
values in the NYC_CONFIG env.  It is not deleted in the child process,
so will be contagious unless unset, and it is the responsibility of the
caller to manage.

The first intended use case of this feature is to allow node-tap to map
tests to the specific portion of a system under test that they cover.
In this way, unit tests can be more focused, and only re-run when their
specific unit has changed.

There are, of course, many other uses that this could be put to, and it
did not seem appropriate to add a special hook _just_ for overriding
the include list.
@coreyfarrell
Copy link
Member

I'm concerned about our expanding environmental variable surface. Not specifically against this but I'd like to hear what @JaKXz and @bcoe have to say / if they can think of any alternative methods of accomplishing the goal.

@isaacs
Copy link
Collaborator Author

isaacs commented Apr 19, 2019

@coreyfarrell #1078 removes some environs that are no longer load-bearing, if you're concerned about that.

@coreyfarrell
Copy link
Member

@isaacs I think removing unused environs in the next major release will be good but my concern is more about adding environmental variables which can be set from outside NYC to manipulate how it runs.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This is looking good to me, excited to see what you do with it.

Perhaps you can coordinate with @coreyfarrell (and his work here) to add the new tests as snapshots?

Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

I echo @coreyfarrell's concern but I don't feel strongly enough about it to block the merge.

I appreciate the approach of adding new test files for new features!

@coreyfarrell coreyfarrell merged commit 132a074 into istanbuljs:master Apr 24, 2019
@isaacs isaacs deleted the conf-override branch April 24, 2019 18:38
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.

None yet

4 participants