Skip to content

[cli] add telemetry tracking to project add#12340

Merged
kodiakhq[bot] merged 3 commits intomainfrom
austinmerrick/zero-2685-add-telemetry-tracking-for-vercel-project-add
Oct 23, 2024
Merged

[cli] add telemetry tracking to project add#12340
kodiakhq[bot] merged 3 commits intomainfrom
austinmerrick/zero-2685-add-telemetry-tracking-for-vercel-project-add

Conversation

@onsclom
Copy link
Contributor

@onsclom onsclom commented Oct 22, 2024

Telemetry for vc project add:

1 argument (name)
0 flags
0 options

@changeset-bot
Copy link

changeset-bot bot commented Oct 22, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 68f24f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
vercel Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@onsclom onsclom marked this pull request as ready for review October 22, 2024 19:38
jeffsee55
jeffsee55 requested changes Oct 22, 2024
View reviewed changes
packages/cli/test/unit/commands/project/add.test.ts
const project: Project = await client.fetch(`/v8/projects/test-project`);
expect(project).toBeDefined();

expect(client.telemetryEventStore).toHaveTelemetryEvents([
Copy link
Contributor

@jeffsee55 jeffsee55 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of creating a new test that doesn't differ from the one above you can just put this assertion into the first test

Copy link
Contributor Author

@onsclom onsclom Oct 22, 2024

Choose a reason for hiding this comment

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

Good point, was copying this pattern from https://github.com/vercel/vercel/pull/12194/files#diff-e6a0a76a9158df1b8578fa4bbd0a67a25019133076cdae07cc0dfeb5e99c8695R40. Since this pattern already exists and it provides better test failing messages, maybe we keep it like that?

Copy link
Contributor

@trek trek Oct 23, 2024

Choose a reason for hiding this comment

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

Lemme short circuit the conversation since I swear it always goes exactly like this:

"Or shift the duplicated code to a beforeEach and only focus on the expectations of client.telemetryEventStore."
"Then we're just running more tests and that takes more time."
"But we get focused failures! Can we just use beforeAll"
"beforeAll just ends up causing really confusing state issues that are hard to debug"

Copy link
Contributor

@EndangeredMassa EndangeredMassa Oct 23, 2024

Choose a reason for hiding this comment

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

This is a unit test that appears to hit mocked endpoints. I imagine it's super fast. If true, I don't think we need to worry about adding an extra test here.

Copy link
Contributor

@trek trek Oct 23, 2024

Choose a reason for hiding this comment

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

I gave my approval because both options are correct but also wrong.

Copy link
Contributor

@trek trek Oct 23, 2024

Choose a reason for hiding this comment

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

@EndangeredMassa it still takes time to set create all those objects and execute the code under test! Eventually it adds up.

Copy link
Contributor

@trek trek Oct 23, 2024

Choose a reason for hiding this comment

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

(stage whisper) Sean, it's your line.

Copy link
Contributor

@jeffsee55 jeffsee55 Oct 23, 2024

Choose a reason for hiding this comment

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

In general, the patterns I've come to like in tests:

  • Use beforeAll is little as possible for reasons Trek already stated.
  • Arrange, Act, Assert is a nice mental model, and beforeEach should only be used for the Arrange
  • Multiple assertions are good. They are a way of saying, when this thing happens, here are all of the side effects or outcomes. You can read them in one place. I hadn't given much thought to the fact that you get a better error message when you split them up, good point @onsclom

Copy link
Contributor

@trek trek Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jeffsee55 it also depends on the framework. Some have expect syntax like expect(actual).toSomethingSomething(expected, failureExplanationIfNot) or similar. @alex_neo/jest-expect-message has this for jest so should work with vite as well I think?

If we had and used that consistently I'd be happy with many assertions. Plus it lets you add better failure reasons generally.

Good:

it('project user count and team user account identical', () => {
  project.addUser(user);
  expect(project.userCount).toEqual(team.userCount);
});
// "Failed 'project user count and team user account identical'. Expected 3, got 2" 
// is super actionable.

Bad:

it('adding a user', () => {
  project.addUser(user);
  expect(project.userCount).toEqual(team.userCount);
});
// "Failed 'adding a user'. Expected 3, got 2" 
// requires my faulty skull bacon to do work

But, also good:

it('adding a user', () => {
  project.addUser(user);
  expect(project.userCount, 'project user count and team user account must be identical').toEqual(team.userCount);
});
// "Failed 'adding a user': Project user count and team user account must be identical. Expected 3, got 2"
// back to actionable.

trek
trek reviewed Oct 23, 2024
View reviewed changes
packages/cli/test/unit/commands/project/add.test.ts
client.setArgv('project', 'add', 'test-project');
await projects(client);

const project: Project = await client.fetch(`/v8/projects/test-project`);
Copy link
Contributor

@trek trek Oct 23, 2024

Choose a reason for hiding this comment

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

I know this was copied from above but... what is it doing?

Copy link
Contributor Author

@onsclom onsclom Oct 23, 2024

Choose a reason for hiding this comment

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

Good catch! Looks like it's just testing the useProject mock. I'll make a followup PR to remove this

@kodiakhq kodiakhq bot merged commit 804b94f into main Oct 23, 2024
@kodiakhq kodiakhq bot deleted the austinmerrick/zero-2685-add-telemetry-tracking-for-vercel-project-add branch October 23, 2024 22:18
@vercel-release-bot vercel-release-bot mentioned this pull request Oct 23, 2024
Merged
@onsclom onsclom mentioned this pull request Oct 23, 2024
Merged
trek pushed a commit that referenced this pull request Oct 24, 2024
@onsclom @trek
[cli] add telemetry tracking to project add (#12340)
1f030e7 
Telemetry for `vc project add`:

1 argument (name)
0 flags
0 options
kodiakhq bot pushed a commit that referenced this pull request Oct 24, 2024
@onsclom
Remove asserts on mock function (#12359)
7129682 
These asserts only test the `useProject` mock, not `project add` logic. Thanks for catching this @trek! 

#12340 (comment)
onsclom pushed a commit that referenced this pull request Oct 29, 2024
@vercel-release-bot @github-actions
Version Packages (#12345)
bd7d2e7 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## vercel@37.13.0

### Minor Changes

- Add telemetry for `vercel logs`
([#12386](#12386))

- Add telemetry for `vercel env pull`
([#12368](#12368))

- add telemetry for `vercel env rm`
([#12384](#12384))

- [cli] add subcommand tracking for `dns` group
([#12375](#12375))

- [cli] add telemetry for `vercel certs ls`
([#12383](#12383))

- [cli] add telemetry tracking to vercel inspect
([#12367](#12367))

- Add telemetry for `vercel certs remove`
([#12394](#12394))

- Add telemetry for `vercel integration list`
([#12404](#12404))

- add telemetry tracking to `env add`
([#12357](#12357))

- track standard environments in `vc env add`
([#12385](#12385))

- [cli] add telemetry for `vercel target ls`
([#12352](#12352))

- [cli] add telemetry tracking for `vercel domains ls`
([#12400](#12400))

- [cli] add `VERCEL_ENV` and `VERCEL` to process to simulate runtime
([#12358](#12358))

- Add telemetry for `vercel domains move`
([#12411](#12411))

- [cli] add telemetry tracking to `git connect` and `git disconnect`
([#12373](#12373))

- Add telemetry to `vercel domains buy`
([#12413](#12413))

- [cli] add telemetry tracking to `project list`
([#12339](#12339))

- Add telemetry for `vercel domains add`
([#12409](#12409))

- Add telemetry to `vercel domains rm`
([#12410](#12410))

- Add telemetry for `vercel list`
([#12364](#12364))

- [cli] add subcommand tracking for `integration` group
([#12377](#12377))

- Add telemetry for `vercel certs add`
([#12391](#12391))

- [cli] add subcommand tracking for `domains` group
([#12374](#12374))

- [cli] add telemetry tracking to `project add`
([#12340](#12340))

- Add telemetry for `vercel integration open`
([#12408](#12408))

- Add telemetry for `vercel init`
([#12371](#12371))

- [cli] add subcommand tracking for `integration` group
([#12377](#12377))

- [cli] add telemetry tracking for `vercel dns import`
([#12379](#12379))

- [cli] add telemetry for `vercel dns ls`
([#12380](#12380))

- Add telemetry for `vercel env ls`
([#12392](#12392))

- Add telemetry for `vercel bisect`
([#12362](#12362))

- [cli] add telemetry for `vercel dns remove`
([#12381](#12381))

- Add telemetry for `vercel integration add`
([#12406](#12406))

- [cli] add telemetry tracking for vercel dev invocations
([#12349](#12349))

- [cli] add subcommand tracking for `certs` group
([#12376](#12376))

- [cli] add telemetry for `vercel redeploy`
([#12353](#12353))

- [cli] add telemetry for `vercel domains inspect`
([#12407](#12407))

- Add telemetry for `vercel certs issue`
([#12401](#12401))

- Add telemetry for `vercel link`
([#12360](#12360))

### Patch Changes

- Remove stray file
([#12363](#12363))

- add metrics to dns add
([#12414](#12414))

- Add telemetry to the `teams` subcommands. Telemetry collection is not
currently enabled and when it is, will be a major version bump for the
CLI. ([#12346](#12346))

- add telemetry to `vc pull`
([#12387](#12387))

- [cli] refactor Output usage
([#12305](#12305))

- extract pull invocation from build
([#12372](#12372))

- extract vc pull logic from entrypoint
([#12378](#12378))

- Updated dependencies
\[[`13bb443f2b0791c7bd402447fcb540cffd0b6eae`](13bb443)]:
    -   @vercel/remix-builder@2.2.13

## @vercel/remix-builder@2.2.13

### Patch Changes

- Update `@remix-run/dev` fork to v2.13.1
([#12334](#12334))

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
QuietCraftsmanship pushed a commit to QuietCraftsmanship/Vercel that referenced this pull request Jul 6, 2025
@onsclom
Remove asserts on mock function (#12359)
a87207c 
These asserts only test the `useProject` mock, not `project add` logic. Thanks for catching this @trek! 

vercel/vercel#12340 (comment)
QuietCraftsmanship pushed a commit to QuietCraftsmanship/Vercel that referenced this pull request Jul 6, 2025
@github-actions
Version Packages (#12345)
8067a88 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## vercel@37.13.0

### Minor Changes

- Add telemetry for `vercel logs`
([#12386](vercel/vercel#12386))

- Add telemetry for `vercel env pull`
([#12368](vercel/vercel#12368))

- add telemetry for `vercel env rm`
([#12384](vercel/vercel#12384))

- [cli] add subcommand tracking for `dns` group
([#12375](vercel/vercel#12375))

- [cli] add telemetry for `vercel certs ls`
([#12383](vercel/vercel#12383))

- [cli] add telemetry tracking to vercel inspect
([#12367](vercel/vercel#12367))

- Add telemetry for `vercel certs remove`
([#12394](vercel/vercel#12394))

- Add telemetry for `vercel integration list`
([#12404](vercel/vercel#12404))

- add telemetry tracking to `env add`
([#12357](vercel/vercel#12357))

- track standard environments in `vc env add`
([#12385](vercel/vercel#12385))

- [cli] add telemetry for `vercel target ls`
([#12352](vercel/vercel#12352))

- [cli] add telemetry tracking for `vercel domains ls`
([#12400](vercel/vercel#12400))

- [cli] add `VERCEL_ENV` and `VERCEL` to process to simulate runtime
([#12358](vercel/vercel#12358))

- Add telemetry for `vercel domains move`
([#12411](vercel/vercel#12411))

- [cli] add telemetry tracking to `git connect` and `git disconnect`
([#12373](vercel/vercel#12373))

- Add telemetry to `vercel domains buy`
([#12413](vercel/vercel#12413))

- [cli] add telemetry tracking to `project list`
([#12339](vercel/vercel#12339))

- Add telemetry for `vercel domains add`
([#12409](vercel/vercel#12409))

- Add telemetry to `vercel domains rm`
([#12410](vercel/vercel#12410))

- Add telemetry for `vercel list`
([#12364](vercel/vercel#12364))

- [cli] add subcommand tracking for `integration` group
([#12377](vercel/vercel#12377))

- Add telemetry for `vercel certs add`
([#12391](vercel/vercel#12391))

- [cli] add subcommand tracking for `domains` group
([#12374](vercel/vercel#12374))

- [cli] add telemetry tracking to `project add`
([#12340](vercel/vercel#12340))

- Add telemetry for `vercel integration open`
([#12408](vercel/vercel#12408))

- Add telemetry for `vercel init`
([#12371](vercel/vercel#12371))

- [cli] add subcommand tracking for `integration` group
([#12377](vercel/vercel#12377))

- [cli] add telemetry tracking for `vercel dns import`
([#12379](vercel/vercel#12379))

- [cli] add telemetry for `vercel dns ls`
([#12380](vercel/vercel#12380))

- Add telemetry for `vercel env ls`
([#12392](vercel/vercel#12392))

- Add telemetry for `vercel bisect`
([#12362](vercel/vercel#12362))

- [cli] add telemetry for `vercel dns remove`
([#12381](vercel/vercel#12381))

- Add telemetry for `vercel integration add`
([#12406](vercel/vercel#12406))

- [cli] add telemetry tracking for vercel dev invocations
([#12349](vercel/vercel#12349))

- [cli] add subcommand tracking for `certs` group
([#12376](vercel/vercel#12376))

- [cli] add telemetry for `vercel redeploy`
([#12353](vercel/vercel#12353))

- [cli] add telemetry for `vercel domains inspect`
([#12407](vercel/vercel#12407))

- Add telemetry for `vercel certs issue`
([#12401](vercel/vercel#12401))

- Add telemetry for `vercel link`
([#12360](vercel/vercel#12360))

### Patch Changes

- Remove stray file
([#12363](vercel/vercel#12363))

- add metrics to dns add
([#12414](vercel/vercel#12414))

- Add telemetry to the `teams` subcommands. Telemetry collection is not
currently enabled and when it is, will be a major version bump for the
CLI. ([#12346](vercel/vercel#12346))

- add telemetry to `vc pull`
([#12387](vercel/vercel#12387))

- [cli] refactor Output usage
([#12305](vercel/vercel#12305))

- extract pull invocation from build
([#12372](vercel/vercel#12372))

- extract vc pull logic from entrypoint
([#12378](vercel/vercel#12378))

- Updated dependencies
\[[`e4304c4526f6062d23b7fdb381d9bb24c7431a0d`](vercel/vercel@e4304c4)]:
    -   @vercel/remix-builder@2.2.13

## @vercel/remix-builder@2.2.13

### Patch Changes

- Update `@remix-run/dev` fork to v2.13.1
([#12334](vercel/vercel#12334))

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffsee55 jeffsee55 jeffsee55 approved these changes

@TooTallNate TooTallNate Awaiting requested review from TooTallNate

@EndangeredMassa EndangeredMassa Awaiting requested review from EndangeredMassa

@erikareads erikareads Awaiting requested review from erikareads

+1 more reviewer

@trek trek trek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@onsclom @trek @EndangeredMassa @jeffsee55