-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: remove node globals #46
fix: remove node globals #46
Conversation
This PR remove node globals, updates deps and fixes lint problems. related to this ipfs/js-ipfs#2924
test/garbage.spec.js
Outdated
const CBOR_GARBAGE = process.env.NODE_CBOR_GARBAGE | ||
const NO_GARBAGE = process.env.NO_GARBAGE | ||
const REPEATS = parseInt(CBOR_GARBAGE || 1000) | ||
// const CBOR_GARBAGE = global.process.env.NODE_CBOR_GARBAGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just removing those instead of commenting out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wanted to confirm with the reviewer if this would be better removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to be able to set these values when testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure but these are tests and nothing is setting these env vars
how would you like this to be fixed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it's neither document nor CI is using it. When is this test ever skipped (NO_GARBAGE
is set)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test does nothing probably some left over from history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this, and manually set it when I working on a fast iteration cycle as this is a pertty slow test. Please preserve the functionality that is there, allowing configuration of this test through setting env variables
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
=========================================
Coverage ? 82.79%
=========================================
Files ? 9
Lines ? 1081
Branches ? 0
=========================================
Hits ? 895
Misses ? 186
Partials ? 0 Continue to review full report at Codecov.
|
should be good to go and ci is passing https://travis-ci.org/github/dignifiedquire/borc/builds/666838072 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dignifiedquire can you do the merge and release stuff if you approve?
@dignifiedquire can you release please ? |
This PR remove node globals, updates deps and fixes lint problems.
related to this ipfs/js-ipfs#2924