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

Set NODE_ENV to to 'test' if not already set #1523

Merged
merged 4 commits into from Oct 1, 2017

Conversation

Projects
None yet
2 participants
@P-Seebauer
Contributor

P-Seebauer commented Sep 16, 2017

Fixes #1470

Still needs tests and docs.

I haven't found a good place in the docs to put the "warnings about the dangers".

Also stops process.env.AVA_PATH from getting overwritten in fork.js. I did not find a reason why the Object.assign is inside the if statement. (I have had a look at #559 and the likes and it seems to me that there is a similar issue with the AVA_PATH waiting to happen). Since there are no tests, I believe that the overwrite was not intentional (and could move to another place, if it was).

@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn

novemberborn Sep 17, 2017

Member

Thanks @P-Seebauer!

I haven't found a good place in the docs to put the "warnings about the dangers".

Perhaps https://github.com/avajs/ava/#process-isolation comes closest, as it talks about the test process. Add another paragraph explaining what value of NODE_ENV to expect, and warning users that this may influence their code?

Also stops process.env.AVA_PATH from getting overwritten in fork.js. I did not find a reason why the Object.assign is inside the if statement. (I have had a look at #559 and the likes and it seems to me that there is a similar issue with the AVA_PATH waiting to happen). Since there are no tests, I believe that the overwrite was not intentional (and could move to another place, if it was).

I think you're right. I must've assumed it was a copy already. The value isn't used in the main process though so it was harmless.

Member

novemberborn commented Sep 17, 2017

Thanks @P-Seebauer!

I haven't found a good place in the docs to put the "warnings about the dangers".

Perhaps https://github.com/avajs/ava/#process-isolation comes closest, as it talks about the test process. Add another paragraph explaining what value of NODE_ENV to expect, and warning users that this may influence their code?

Also stops process.env.AVA_PATH from getting overwritten in fork.js. I did not find a reason why the Object.assign is inside the if statement. (I have had a look at #559 and the likes and it seems to me that there is a similar issue with the AVA_PATH waiting to happen). Since there are no tests, I believe that the overwrite was not intentional (and could move to another place, if it was).

I think you're right. I must've assumed it was a copy already. The value isn't used in the main process though so it was harmless.

@P-Seebauer P-Seebauer changed the title from [WIP] Set NODE_ENV to to 'test' if not already set to Set NODE_ENV to to 'test' if not already set Sep 17, 2017

@P-Seebauer

This comment has been minimized.

Show comment
Hide comment
@P-Seebauer

P-Seebauer Sep 18, 2017

Contributor

I think, I'm done.

Sorry about the commit/build spam, I wasn't aware that github links the branch this way.

I'm not too happy about the Documentation part. Do you have any suggestions what could be done better?

Contributor

P-Seebauer commented Sep 18, 2017

I think, I'm done.

Sorry about the commit/build spam, I wasn't aware that github links the branch this way.

I'm not too happy about the Documentation part. Do you have any suggestions what could be done better?

@novemberborn

I think, I'm done.

👍

I'm not too happy about the Documentation part. Do you have any suggestions what could be done better?

See the inline comment below.

Show outdated Hide outdated readme.md

novemberborn added some commits Oct 1, 2017

@novemberborn novemberborn merged commit 42e7c74 into avajs:master Oct 1, 2017

4 checks passed

codecov/patch 100% of diff hit (target 97.01%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +2.98% compared to 8955e15
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn
Member

novemberborn commented Oct 1, 2017

Thanks @P-Seebauer!

@P-Seebauer P-Seebauer deleted the P-Seebauer:set-node_env branch Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment