Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

test harness should be able to expect errors #319

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion cmd/dep/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
return err
}
if mok {
return fmt.Errorf("manifest file %q already exists", mf)
return errors.New("manifest file already exists")
}
// Manifest file does not exist.

Expand Down
12 changes: 10 additions & 2 deletions cmd/dep/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,18 @@ func TestIntegration(t *testing.T) {

// Run commands
testProj.RecordImportPaths()
for _, args := range testCase.Commands {
testProj.DoRun(args)

var err error
for i, args := range testCase.Commands {
err = testProj.DoRun(args)
if err != nil && i < len(testCase.Commands)-1 {
t.Fatalf("cmd %s raised an unexpected error: %s", args[0], err.Error())
}
}

// Check errors from final command
testCase.CompareError(err, testProj.GetStderr())

// Check final manifest and lock
testCase.CompareFile("manifest.json", testProj.ProjPath("manifest.json"))
testCase.CompareFile("lock.json", testProj.ProjPath("lock.json"))
Expand Down
12 changes: 7 additions & 5 deletions cmd/dep/testdata/harness_tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ The `testcase.json` file has the following format:
"github.com/sdboyer/deptestdos",
"github.com/sdboyer/deptesttres",
"github.com/sdboyer/deptestquatro"
]
],
"error-expected": "something went wrong"
}

All of the categories are optional - if the `imports` list for a test is empty,
Expand All @@ -72,9 +73,10 @@ The test procedure is as follows:
4. Fetch the repos and versions in `gopath-initial` into `$TMPDIR/src` directory
5. Fetch the repos and versions in `vendor-initial` to the project's `vendor` directory
6. Run `commands` on the project, in declaration order
7. Check the resulting files against those in the `final` input directory
8. Check the `vendor` directory for the projects listed under `vendor-final`
9. Check that there were no changes to `src` listings
10. Clean up
7. Ensure that, if any errors are raised, it is only by the final command and their string output matches `error-expected`
8. Check the resulting files against those in the `final` input directory
9. Check the `vendor` directory for the projects listed under `vendor-final`
10. Check that there were no changes to `src` listings
11. Clean up

Note that for the remote fetches, only git repos are currently supported.
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"commands": [
["init"],
["ensure", "-update", "github.com/sdboyer/deptest"]
],
"vendor-final": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"commands": [
["init"],
["ensure", "-n", "-update", "github.com/sdboyer/deptest"]
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty manifest in final, thought it was needed if we pass the exception test will double check and remove if not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the "don't check a nonexistent file" plan you proposed, in which case its not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

My question was more about the wierd symbol in the Files Changed area. Likely a "no ending newline" meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your right i think I can remove it when i get the non existing file issue sorted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was probably best to keep this just to make sure that the manifest didn't change as it shouldn't have in this scenario.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"commands": [
["init"]
],
"error-expected" : "manifest file already exists"
}
27 changes: 25 additions & 2 deletions test/integration_testcase.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"os"
"path/filepath"
"regexp"
"strings"
"testing"
)

Expand All @@ -27,6 +28,7 @@ type IntegrationTestCase struct {
initialPath string
finalPath string
Commands [][]string `json:"commands"`
ErrorExpected string `json:"error-expected"`
GopathInitial map[string]string `json:"gopath-initial"`
VendorInitial map[string]string `json:"vendor-initial"`
VendorFinal []string `json:"vendor-final"`
Expand Down Expand Up @@ -56,6 +58,26 @@ func NewTestCase(t *testing.T, name string) *IntegrationTestCase {
return n
}

// CompareError compares expected error to error recived.
func (tc *IntegrationTestCase) CompareError(err error, stderr string) {
if err == nil {
return
}

wantExists, want := tc.ErrorExpected != "", tc.ErrorExpected
gotExists, got := stderr != "", stderr

if wantExists && gotExists {
if !strings.Contains(got, want) {
tc.t.Errorf("expected error %s, got error %s", want, got)
}
} else if !wantExists && gotExists {
tc.t.Fatal("error raised where none was expected")
} else if wantExists && !gotExists {
tc.t.Error("error was not raised where one was expected")
}
}

var jsonNils *regexp.Regexp = regexp.MustCompile(`.*: null,.*\r?\n`)
var jsonCmds *regexp.Regexp = regexp.MustCompile(`(?s) "commands": \[(.*) ],`)
var jsonInds *regexp.Regexp = regexp.MustCompile(`(?s)\s*\n\s*`)
Expand Down Expand Up @@ -148,11 +170,12 @@ func (tc *IntegrationTestCase) WriteFile(src string, content string) error {
return err
}

func getFile(path string) (bool, string, error) {
_, err := os.Stat(path)
func getFile(path string) (exists bool, contents string, err error) {
_, err = os.Stat(path)
if err != nil {
return false, "", nil
}

f, err := ioutil.ReadFile(path)
if err != nil {
return true, "", err
Expand Down
4 changes: 4 additions & 0 deletions test/integration_testproj.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ func (p *IntegrationTestProject) RunGit(dir string, args ...string) {
p.h.RunGit(dir, args...)
}

func (p *IntegrationTestProject) GetStderr() string {
return p.h.getStderr()
}

func (p *IntegrationTestProject) GetVendorGit(ip string) {
parse := strings.Split(ip, "/")
gitDir := strings.Join(parse[:len(parse)-1], string(filepath.Separator))
Expand Down
7 changes: 1 addition & 6 deletions test/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,12 +506,7 @@ func (h *Helper) Path(name string) string {
joined = filepath.Join(h.tempdir, name)
}

// Ensure it's the absolute, symlink-less path we're returning
abs, err := filepath.EvalSymlinks(joined)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the best idea, as other tests with potentially different assumptions rely on it. If we really need to drop this for the harness' purposes, then could we introduce a separate method without the symlink call that the harness can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep no problem, am away in Flanders this weekend so will fix it up as soon as I'm home 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the test TestIntegration/init/manifest-exists it doesnt like doing the SymLinks checkas the lock file doesnt exist in final and so the code I removes gives me this error:

lstat /private/var/folders/q9/jmtltdhs5hd8574dpdqgn40c0000gn/T/gotest220558389/src/github.com/golang/notexist/lock.json: no such file or directory

However, adding that file in /github.com/golang/dep/cmd/dep/testdata/harness_tests/init/manifest-exists/final seems abit counter intuative as when the tests ends it shouldnt be there as we are testing that if a manifest already exists for a reason we error before generating a lock file.

If I created a new method we would have to change the line https://github.com/golang/dep/blob/master/cmd/dep/integration_test.go#L60 so that it got to call the new method that handled the files not existing.

We could however just return an empty string if we got an error in from EvalSymlinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good to have a bit of a shove in the right direction with how to choose to fix this one up.

if err != nil {
h.t.Fatalf("%+v", errors.Wrapf(err, "internal testsuite error: could not get absolute path for dir(%q)", joined))
}
return abs
return joined
}

// MustExist fails if path does not exist.
Expand Down