fix dep silently failing

[domgreen]

Have tested locally by running go test in the source dirs and also testing manually by using ../../../../bin/dep ensure in a test repo.

Fixes #241

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[domgreen]

I signed it! 👍

[googlebot]

CLAs look good, thanks!

[sdboyer]

looks pretty good! i would like to see some test cases that cover this, though. it might be best to wait until #287 goes in, though (which will hopefully be tonight or tomorrow), as that will provide a more structured way of creating such tests.

[sdboyer]

No need for Errorf() when there's no variables being interpolated happening. Just errors.New() is fine

[sdboyer]

(Also, no need to include the issue number in the commit message)

[domgreen]

Thanks @sdboyer , I did try to drive the functionality via tests but got a little lost. Will wait for the test improvements and give it another bash. Thanks.

[domgreen]

So the reason I was unsure on the tests is that in the ensure_test.go file the tests either seemed to test a single method TestDeduceConstraint or used the file system to run the tests (refactoring being done in #287 ).

My natural instinct would be to always test from the public Run func as this seems to be the entry point for the behaviour then fake out unneeded components.

Could you advise me on how best to test this bug fix? Sorry if I'm overcomplicating things.

[sdboyer]

@domgreen (new testing harness is in)

My natural instinct would be to always test from the public Run func as this seems to be the entry point for the behaviour then fake out unneeded components.

For the most part, we've been doing full-on integration tests here. They have mostly the same effect as calling that Run func, except that it's happening in a sub-process. (We do have some more unit-ish tests, but because so much of the meat of the logic here is in gps, there's less value in more unit-scale tests.)

Have a look at the docs for writing a test using the new harness. Basically, you need to put down a certain pattern of files on disk.

[domgreen]

No problem will follow that pattern, I did look at the new code which looks straight foward in most cases. I will see how I can get the error message displayed from the output. As I expect that I would want to assert that the error is present as well as the file system being in the correct state.

[tro3]

That is a test harness "feature" that definitely needs to be added - expected error cases. (Right now, the few there are still code-based tests.) It's on my todo list, but if you want to take a crack at it, let me know.

[domgreen]

I didn't mean to close :\ having git issues after rebasing master onto my fork

[domgreen]

So, case3 to me doesn't really test what I wanted to test ... as @tro3 says maybe I should look into the test harness to see how I can get it to expect errors 👍

This isn't a lunch time jobbie, will get some time either tonight or tomorrow to see if I can add that in.

[tro3]

Capturing the stdout is pretty easy - test.DoRun already prints the output if the -logs flag is set. The bigger issues are the parsing and -update options, the latter being why I put it off for a future PR. I may get a couple hours this afternoon and take a crack - I already have it "planned", I just suspect a landmine in the plan.

[domgreen]

Thanks, will read through the code anyway to see what I can grok and think of how I would do it. I have created a new branch for myself as well to have a "tinker".

[tro3]

@domgreen, would you like to carve it up between us? If I work on the -update of the testcase.json, and you work on adding an Error field to the TestCase struct and a test comparison to the code, they would merge cleanly. Then the existing code-based error tests can be migrated as well. Thoughts? I'm also happy to just crank it all out, if preferred.

[domgreen]

I wouldnt mind giving it a bash 😸 I will do it as a new PR, very new to go (this is the first go code ive written) so may pester you abit on the PR.

If that is okay for you then I will look over the weekend.

[domgreen]

@sdboyer would like to pick this up again as well 👍 (will start a new PR)