Skip to content

cli: fix missing dev suffix in version analytics#1488

Merged
jazzdan merged 2 commits into
masterfrom
jazzdan/fix_dev_version_analytics
Apr 12, 2019
Merged

cli: fix missing dev suffix in version analytics#1488
jazzdan merged 2 commits into
masterfrom
jazzdan/fix_dev_version_analytics

Conversation

@jazzdan

@jazzdan jazzdan commented Apr 12, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@jazzdan
jazzdan requested review from landism and nicks April 12, 2019 18:16
Comment thread internal/cli/build.go Outdated
return e == BuildInfo{}
}

func (e BuildInfo) FullVersion() string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we call it analytics version or smth? "full" is very vague

Comment thread internal/cli/build_test.go Outdated
assert.Equal(t, fmt.Sprintf("v%s", devVersion), string(provideWebVersion(provideBuildInfo())))
}

func TestBuildVersion(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not seem like a useful test. It's unlikely to catch an actual regression. A better test might be one that tests what analytics is sending

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(a good rule of thumb : would this test have prevented this bug? In this case, the bug is in the interaction between two components, so a test that only exercises one does not really get at the problem)

@jazzdan
jazzdan merged commit f962179 into master Apr 12, 2019
@nicks
nicks deleted the jazzdan/fix_dev_version_analytics branch February 3, 2022 16:49
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.

2 participants