Skip to content
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

router: Remove fake etcd/discoverd from tests #775

Merged
merged 1 commit into from Jan 20, 2015

Conversation

Projects
None yet
3 participants
@titanous
Copy link
Member

titanous commented Jan 20, 2015

This code is brittle as it makes many assumptions about the behavior of etcd/discoverd, which may not be correct. It was originally added to make the tests faster, but inaccurate tests are useless, no matter how fast.

Closes #771

router: Remove fake etcd/discoverd from tests
This code is brittle as it makes many assumptions about the behavior
of etcd/discoverd, which may not be correct. It was originally added to
make the tests faster, but inaccurate tests are useless, no matter how
fast.

Closes #771

Signed-off-by: Jonathan Rudenberg <jonathan@titanous.com>
@lmars

This comment has been minimized.

Copy link
Member

lmars commented Jan 20, 2015

LGTM

titanous added a commit that referenced this pull request Jan 20, 2015

Merge pull request #775 from flynn/remove-router-fake
router: Remove fake etcd/discoverd from tests

@titanous titanous merged commit 9283fb1 into master Jan 20, 2015

1 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
continuous-integration/flynn The Flynn CI build passed
Details

@titanous titanous deleted the remove-router-fake branch Jan 20, 2015

@bgentry

This comment has been minimized.

Copy link
Contributor

bgentry commented Jan 20, 2015

FYI this broke tests locally for me until I manually did a go install on discoverd. Saw the following in my router test logs with DEBUG=1:

/Users/bgentry/go/src/github.com/flynn/flynn/discoverd/testutil/testutil.go:34:
    t.Fatal("discoverd start failed: ", err)
... Error: discoverd start failed: exec: "discoverd": executable file not found in $PATH

Should I be running the router tests via make instead of go test ?

@titanous

This comment has been minimized.

Copy link
Member Author

titanous commented Jan 20, 2015

The new make test-unit target should work inside the dev VM, but will run all of the tests, not just the router tests. As long as you have discoverd/etcd in your PATH, go test is fine (and works on OS X as well as Linux).

@bgentry

This comment has been minimized.

Copy link
Contributor

bgentry commented Jan 20, 2015

Running the tests with go test does "work", although it seems to be execing etcd and discoverd for each individual test, which means the router tests now take ~26s locally :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.