-
-
Notifications
You must be signed in to change notification settings - Fork 650
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
Fail fast when the outputdir of combined coverage profile does not exist #446
Conversation
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 seems like a sensible tradeoff. The behaviour has changed, but only because the go test
behaviour has changed. Keeping the old behaviour would have added complexity without benefit.
I would like to see a comment added to the change log now, because it will get forgotten later.
integration/coverage_test.go
Outdated
@@ -112,17 +112,12 @@ var _ = Describe("Coverage Specs", func() { | |||
os.RemoveAll("./_fixtures/combined_coverage_fixture/coverage.txt") | |||
}) | |||
|
|||
It("Creates directories in path if they don't exist", func() { | |||
It("Fails with an error if cannot combine the suite coverage profiles because the output dir does not exist", func() { |
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.
Could we possible put this in a Describe
?
Describe("when the output dir does not exist", func() {
It("fails with an error", func() {
I don't think the text "cannot combine the suite coverage profiles" adds very much, so I would omit it.
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 reason I specified "cannot combine the suite coverage profiles" is that by omitting the -coverageprofile
flag, ginkgo will try to move the separate coverage profiles into the new directory without combining them.
As far as I understand, this is handled in another function (see here, which has also different tests.
How can we change the text to be clearer and explain that this test is specifically about -coverageprofile
flag?
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.
Describe("when combining coverage profiles", func() {
Describe("when the output dir does not exist", func() {
It("fails with an error", func() {
A very long It()
is a smell, but I'm not sure this is any better.
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.
As an aside from the general discussion, I'd suggest using Context(
in the case where the thing under test is undergoing different preconditions rather than Describe(
. In fact we also have When(
which is an alias for Context(
and prefixes when
to the description.
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.
@blgm I have changed the description to be "Fails with an error if output dir and coverprofile were set, but the output dir did not exist", which follows the same structure to the It
above.
In my opinion, adding a level of nesting such as Context
, When
, or Describe
would make the tests less readable, but I agree that it was a very long It
description.
What do you think about the message now?
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.
@nodo, I think the message is clear, and definitely good enough. I take your point regarding nesting. I think it can lead to clearer code, but it would be out of step with the rest of the file, and consistency is more important.
Thanks for your review @blgm ! Adding an item to the changelog is definitely a good idea. I'll do it. |
This is a workaround for a change in behaviour between go <= 1.9 and go 1.10. Prior to go 1.9, compiling a test would not fail if the specified output dir does not exist. However, it does fail in go 1.10. By failing when the the directory does not exist, we have a consistent behaviour withing go versions. See also golang/go#24588
2b2e195
to
c0b857d
Compare
Mention the breaking change when `-coverprofile` and `-outputdir` flags are set, but the directory does not exist.
@blgm updated the changelog, do you think that's enough? |
Looks good to me |
I'm not considering this a breaking change because the |
This is a workaround for a change in behaviour between go <= 1.9 and go
1.10.
Prior to go 1.9, compiling a test would not fail if the specified
output dir does not exist. However, it does fail in go 1.10.
By failing when the the directory does not exist, we have a consistent
behaviour withing go versions.
See also golang/go#24588