-
Notifications
You must be signed in to change notification settings - Fork 182
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(cli): don't run analytics during tests #84
Conversation
module.exports = { | ||
ANALYTICS_KEY, | ||
ANALYTICS_MODES, | ||
API_PATH, | ||
AUTH_KEY, | ||
AUTH_LOCATION, |
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.
these are just sorting
// '/Users/david/projects/zapier/platform/node_modules/.bin/mocha', | ||
// 'src/tests' ] | ||
const argvStr = process.argv.join(' '); | ||
const IS_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.
we have the same thing in core, helps us to be consistent
@@ -225,6 +225,14 @@ class ZapierBaseCommand extends Command { | |||
.join('\n') | |||
.trim(); | |||
} | |||
|
|||
_recordAnalytics() { |
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 could be overkill, but I did this before finding the bug in analytics.js
itself. In any case, it's nice to centralize the behavior.
|
||
describe('testing setup', () => { | ||
it('should set IS_TESTING to true', () => { | ||
IS_TESTING.should.be.true(); |
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.
simple canary
describe('analytics', () => { | ||
// causes a lot of noise | ||
it('should not run analytics when testing', () => { | ||
shouldSkipAnalytics().should.be.true(); |
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 makes the functionality tough to test. it fixes the problem for now, but we'll probably have to rethink this approach.
@@ -15,19 +15,22 @@ const setAnalyticsMode = newMode => { | |||
return writeUserConfig({ [ANALYTICS_KEY]: newMode }); | |||
}; | |||
|
|||
const shouldSkipAnalytics = mode => |
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.
easier to read this way
const recordAnalytics = async (command, isValidCommand, args, flags) => { | ||
const analyticsMode = await currentAnalyticsMode(); | ||
|
||
const shouldRecordAnalytics = | ||
process.env.DISABLE_ZAPIER_ANALYTICS || | ||
(process.NODE_ENV !== 'test' && analyticsMode !== ANALYTICS_MODES.disabled); |
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 actual bug was having process.NODE_ENV
instead of process.env.NODE_ENV
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 didn't pull but the code looks good to me!
I noticed a big surge of analytics on
linux
with odd behavior, such as no command (which should never be possible). Turns out the culprit was traivs. Fixed some minor bugs while I was in there.