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

add usage tracking for clicking the `ChangedFilesCount` component #1658

Merged
merged 7 commits into from Aug 23, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Contributor

annthurium commented Aug 22, 2018

Description of the Change

In #1573, we discussed discoverability of the Atom package. @simurai proposed changing the status bar tile to say "Git" instead of "n file(s)". I think that's a really good idea, but before implementing that change, I'd like to instrument this component and establish a baseline. That way we'll know if there's a correlation between changing the text and how often it's clicked, even if causation is harder to establish. :-p

This pull request also adds unit tests for ChangedFilesCountView since there previously were none.

Alternate Designs

Based on conversations with @telliott27, I'm using the addEvent api instead of the incrementCounter api. In the past, client apps has used counters for feature usage. However, counters don't retain metadata for each individual event, so addEvent gives us a fuller picture of what's going on.

Benefits

The ability to understand how users are interacting with this component. More broadly, I want to understand if users are having trouble discovering the github package.

Possible Drawbacks

None that I can think of.

Applicable Issues

#1573

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2018

Coverage Status

Coverage decreased (-0.04%) to 80.159% when pulling 24367d5 on tt-18-aug-entry-points into 7e62b14 on master.

coveralls commented Aug 22, 2018

Coverage Status

Coverage decreased (-0.04%) to 80.159% when pulling 24367d5 on tt-18-aug-entry-points into 7e62b14 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2018

Coverage Status

Coverage increased (+0.04%) to 80.164% when pulling 57ab7df on tt-18-aug-entry-points into 784c17b on master.

coveralls commented Aug 22, 2018

Coverage Status

Coverage increased (+0.04%) to 80.164% when pulling 57ab7df on tt-18-aug-entry-points into 784c17b on master.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 23, 2018

Member

This pull request also adds unit tests for ChangedFilesCountView since there previously were none.

Yesss 🥇

So somewhat awkwardly, a bunch of the tests for the status bar components are smashed up into test/controllers/status-bar-tile-controller.test.js. There are a few for the change files view:

describe('changed files', function() {
it('shows the changed files count view when the repository data is loaded', async function() {
const workdirPath = await cloneRepository('three-files');
const repository = await buildRepository(workdirPath);
const toggleGitTab = sinon.spy();
const wrapper = await mountAndLoad(buildApp({repository, toggleGitTab}));
assert.equal(wrapper.find('.github-ChangedFilesCount').render().text(), '0 files');
fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n');
fs.unlinkSync(path.join(workdirPath, 'b.txt'));
await repository.stageFiles(['a.txt']);
repository.refresh();
await assert.async.equal(wrapper.find('.github-ChangedFilesCount').render().text(), '2 files');
});
it('toggles the git panel when clicked', async function() {
const workdirPath = await cloneRepository('three-files');
const repository = await buildRepository(workdirPath);
const toggleGitTab = sinon.spy();
const wrapper = await mountAndLoad(buildApp({repository, toggleGitTab}));
wrapper.find(ChangedFilesCountView).simulate('click');
assert(toggleGitTab.calledOnce);
});
});

But they're pretty sparse and quite old. I'd say delete those in favor of yours ✂️

Member

smashwilson commented Aug 23, 2018

This pull request also adds unit tests for ChangedFilesCountView since there previously were none.

Yesss 🥇

So somewhat awkwardly, a bunch of the tests for the status bar components are smashed up into test/controllers/status-bar-tile-controller.test.js. There are a few for the change files view:

describe('changed files', function() {
it('shows the changed files count view when the repository data is loaded', async function() {
const workdirPath = await cloneRepository('three-files');
const repository = await buildRepository(workdirPath);
const toggleGitTab = sinon.spy();
const wrapper = await mountAndLoad(buildApp({repository, toggleGitTab}));
assert.equal(wrapper.find('.github-ChangedFilesCount').render().text(), '0 files');
fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n');
fs.unlinkSync(path.join(workdirPath, 'b.txt'));
await repository.stageFiles(['a.txt']);
repository.refresh();
await assert.async.equal(wrapper.find('.github-ChangedFilesCount').render().text(), '2 files');
});
it('toggles the git panel when clicked', async function() {
const workdirPath = await cloneRepository('three-files');
const repository = await buildRepository(workdirPath);
const toggleGitTab = sinon.spy();
const wrapper = await mountAndLoad(buildApp({repository, toggleGitTab}));
wrapper.find(ChangedFilesCountView).simulate('click');
assert(toggleGitTab.calledOnce);
});
});

But they're pretty sparse and quite old. I'd say delete those in favor of yours ✂️

@annthurium

This comment has been minimized.

Show comment
Hide comment
@annthurium

annthurium Aug 23, 2018

Contributor

@smashwilson some of the tests in status-bar-tile-controller.test.js are exercising functionality that couldn't be tested by the ChangedFilesCountView in isolation, without breaking encapsulation. For example, testing that clicking the ChangedFilesCountView actually toggles the tab, or testing that the status tile icons don't show up if the project lacks a repository.

There is one test in status-bar-tile-controller that does feels redundant tho, so I'll remove that one.

Contributor

annthurium commented Aug 23, 2018

@smashwilson some of the tests in status-bar-tile-controller.test.js are exercising functionality that couldn't be tested by the ChangedFilesCountView in isolation, without breaking encapsulation. For example, testing that clicking the ChangedFilesCountView actually toggles the tab, or testing that the status tile icons don't show up if the project lacks a repository.

There is one test in status-bar-tile-controller that does feels redundant tho, so I'll remove that one.

annthurium added some commits Aug 23, 2018

@annthurium annthurium merged commit 7fe9b69 into master Aug 23, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 80.159%
Details

@annthurium annthurium deleted the tt-18-aug-entry-points branch Aug 23, 2018

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