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

Better error reporting from dep ensure when packages have errors #844

Merged
merged 5 commits into from Aug 8, 2017

Conversation

Projects
None yet
4 participants
@grepory
Copy link
Contributor

grepory commented Jul 18, 2017

What does this do / why do we need it?

Running dep ensure would generate unhelpful output when directories had code with errors or no go code. This prints the import path of the package with the build error message.

What should your reviewer look out for in this PR?

If you could make sure that my understanding of this functionality is correct, I'd appreciate it. As I noted in #814:

  • If any directory with go has errors, then we return an error.
  • If no directories have go, return an error.
  • If some directories have go with no errors and directories with no go, do not return an error.
  • If all directories have go with no errors, do not return an error.

Do you need help or clarification on anything?

My integration tests run as I would expect them to run, but if you could make sure that this is a reasonable test, I'd greatly appreciate it!

Which issue(s) does this PR fix?

#814

@grepory grepory requested a review from ibrasho as a code owner Jul 18, 2017

@googlebot googlebot added the cla: yes label Jul 18, 2017

@grepory grepory force-pushed the grepory:dep-ensure-errors branch from 767b428 to c75f16e Jul 18, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 19, 2017

closing/reopening to trigger all tests

@sdboyer sdboyer closed this Jul 19, 2017

@sdboyer sdboyer reopened this Jul 19, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 19, 2017

uuuuuuuuughhhhhh github needs to fix their shit

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Jul 20, 2017

Hahaha sorry Sam. <3

@sdboyer sdboyer closed this Jul 20, 2017

@sdboyer sdboyer reopened this Jul 20, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 20, 2017

IT WENT THROUGH ok now tests

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Jul 20, 2017

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Jul 20, 2017

No idea what's wrong. I'll look at it as soon as I can.

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jul 21, 2017

@grepory
The error is from harness_tests ensure/pkg-errors/case2, you are expecting it to return an error, but it's not.

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Jul 21, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 26, 2017

@grepory yeah, it looks like that case2 might be a bit confused, as it has written outputs but is also expecting errors - that should generally be mutually exclusive. i think, i haven't reviewed in detail :/

running locally SHOULD just be a question of cd cmd/dep && go test, or `cd cmd/dep && go test -run='TestIntegration/ensure/pkg-errors/case2' if you want to target just that case.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 7, 2017

bumpy bump bumpity bump bump ❤️

grepory added some commits Jul 18, 2017

Better error reporting from dep ensure when packages have errors
Running dep ensure would generate unhelpful output when directories had
code with errors or no go code. This prints the import path of the
package with the build error message.
@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Aug 7, 2017

Ok. I rebased, I'm trying to look at the test now. Sorry, we're in the middle of preparing for our first user summit and the launch of our alpha, and I'm in a constant state of "oh god."

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Aug 7, 2017

I definitely do not understand how to write correct integration tests, I think.

@grepory grepory force-pushed the grepory:dep-ensure-errors branch from b068324 to 9db6ab9 Aug 7, 2017

@grepory grepory requested a review from sdboyer as a code owner Aug 7, 2017

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Aug 7, 2017

Oh. I just realized what's going on. My test case used to be passing, because the behavior it was testing was incorrect. So the test is actually wrong. I'm looking at the integration tests now. I finally understand the README. ><

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Aug 7, 2017

While testing this, I came across something troubling.

If I have in my test file something like:

package bar

this definitely doesn't compile

Should that yield an error on dep ensure? It doesn't on my branch or on master.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 7, 2017

like, that's the literal contents of the test file? i'm a bit surprised, but not too terribly so - in general, we do not require that your code be able to compile in order to run dep. we do static analysis, but (for now at least) we do very little - only into the imports (go/parser.ImportsOnly)

@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Aug 7, 2017

Ah okay. Yeah that was almost the literal content of the test file. It did detect an unexpected EOF, though (even while including a comment). So I think it mostly works?

@@ -1,13 +1,16 @@
// Copyright 2016 The Go Authors. All rights reserved.
// Copyright 2017 The Go Authors. All rights reserved.

This comment has been minimized.

@sdboyer

sdboyer Aug 8, 2017

Member

nit: i think the standard thing here is to leave the original copyright date? honestly idk

This comment has been minimized.

@grepory

grepory Aug 8, 2017

Contributor

I think I glossed over this in the rebase. I created this file on my branch before this file was created in a recent merge.

b8f7027

So when I added the copyright, I just put 2017 in it.

I can remove the change if you like.

This comment has been minimized.

@sdboyer

sdboyer Aug 8, 2017

Member

nah, it's fine. i have so many other things to worry about 😄

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 8, 2017

ooook, let's put this in. sorry it took so long! 🎉

@sdboyer sdboyer merged commit b8ecfaa into golang:master Aug 8, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grepory

This comment has been minimized.

Copy link
Contributor

grepory commented Aug 8, 2017

Mostly my fault! Thanks! Hopefully I did not just break dep for everyone. Again. :)

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