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

Don't load .env in test environment #928

Merged
merged 1 commit into from Aug 28, 2018
Merged

Don't load .env in test environment #928

merged 1 commit into from Aug 28, 2018

Conversation

spkjp
Copy link
Contributor

@spkjp spkjp commented Aug 24, 2018

Proposed changes

The ~/.ark/.env doesn't exist on Travis, so it makes sense to not even attempt to load it
when ARK_ENV=test is true.

This fixes an issue I ran into when running tests (core-forger) locally:
screenshot_20180824_175318

Since the container loaded my .env and thus set ARK_P2P_PORT to 4002, which caused process.env.ARK_P2P_PORT || 4000 from core/lib/config/testnet/plugins.js to return the wrong port.

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

@codecov-io
Copy link

codecov-io commented Aug 24, 2018

Codecov Report

Merging #928 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #928      +/-   ##
===========================================
- Coverage    51.65%   51.63%   -0.02%     
===========================================
  Files          432      432              
  Lines         7200     7202       +2     
  Branches       924      925       +1     
===========================================
  Hits          3719     3719              
- Misses        3049     3050       +1     
- Partials       432      433       +1
Impacted Files Coverage Δ
packages/core-container/lib/environment.js 79.31% <100%> (-5.88%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 391dcab...73c07f6. Read the comment docs.

@faustbrian
Copy link
Contributor

If this is a travis specific issue you should use if(process.env.ARK_CI_TEST) because the current check will also be active for testnet.

@spkjp
Copy link
Contributor Author

spkjp commented Aug 26, 2018

@faustbrian It's not an issue with travis, since there's no .env to read.

If the container reads an .env it overwrites the node ports which are hardcoded in unit tests otherwise. Like in core-forger/__tests__/client.test.js, where the p2p port is hardcoded to const host = 'http://127.0.0.1:4000'. It breaks once an .env exists with a different ARK_P2P_PORT.

Another solution would be to change core/lib/config/testnet/plugins.js to only use hardcoded ports.

@faustbrian
Copy link
Contributor

If we no longer load the .env file for testnet the local testing can become quite painful. I have an .env file that I use to store all my testnet configuration that I copy between local and server so I can always have the exact same config.

I will take a closer look when I am back tomorrow.

if (process.env.ARK_ENV === 'test') {
return
}

const envPath = expandHomeDir(`${process.env.ARK_PATH_DATA}/.env`)

if (fs.existsSync(envPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this already prevent the loading of any file if it doesn't exist?

@codecov-io
Copy link

codecov-io commented Aug 28, 2018

Codecov Report

Merging #928 into develop will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #928      +/-   ##
===========================================
- Coverage    52.71%   52.71%   -0.01%     
===========================================
  Files          436      436              
  Lines         7280     7282       +2     
  Branches       940      941       +1     
===========================================
+ Hits          3838     3839       +1     
  Misses        3014     3014              
- Partials       428      429       +1
Impacted Files Coverage Δ
packages/core-container/lib/environment.js 79.31% <100%> (-5.88%) ⬇️
packages/core-p2p/lib/utils/check-ntp.js 100% <0%> (+7.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c85e48a...1bffbf3. Read the comment docs.

@faustbrian faustbrian merged commit 5ea3eb4 into ArkEcosystem:develop Aug 28, 2018
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

3 participants