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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test harness can now assert if errors have been raised correctly #402

Merged
merged 5 commits into from Apr 29, 2017

Conversation

Projects
None yet
3 participants
@domgreen
Copy link
Contributor

domgreen commented Apr 19, 2017

@sdboyer new PR for updating the test harness to have verify errors, with the other update work that has happened the error that I was having earlier has now gone 馃槃

@domgreen domgreen referenced this pull request Apr 19, 2017

Closed

Fix dep silent fail #403

@sdboyer
Copy link
Member

sdboyer left a comment

Sorry, this fell off my radar again :/

Just a small change, then I promise, merge time 馃槃

@@ -73,7 +73,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
return err
}
if mok {
return errors.Errorf("manifest file %q already exists", mf)
return errors.New("manifest file already exists")

This comment has been minimized.

@sdboyer

sdboyer Apr 27, 2017

Member

This isn't the best - we don't want to remove useful information from an error message that's shown to humans just to make the tests pass.

Let's change it to return errors.Errorf("manifest already exists: %s", mf), then change the check from exact equality to strings.Contains().


if wantExists && gotExists {
if !strings.Contains(got, want) {
tc.t.Errorf("expected error %s, got error %s", want, got)

This comment has been minimized.

@sdboyer

sdboyer Apr 27, 2017

Member

Text here will need to change slightly to match the notion of "contains" instead of "equals"

@domgreen

This comment has been minimized.

Copy link
Contributor

domgreen commented Apr 28, 2017

@sdboyer small change done 馃憤

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 29, 2017

@domgreen excellent! thanks so much for your perseverance!

@sdboyer sdboyer merged commit c29d650 into golang:master Apr 29, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 29, 2017

@domgreen also, you should fix your email so your user gets properly associated with these commits 馃槃

@domgreen

This comment has been minimized.

Copy link
Contributor

domgreen commented Apr 29, 2017

@sdboyer oh I thought I had done that 馃樋 take it its something I need to do on my local machine as well as my GitHub profile.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 29, 2017

Looks like you got it 馃槂

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#402 from domgreen/test_harness_errors
Test harness can now assert if errors have been raised correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment