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

Fix unit test runner and failing tests #67

Merged
merged 9 commits into from
Dec 19, 2017
Merged

Conversation

pcalcado
Copy link
Contributor

@pcalcado pcalcado commented Dec 19, 2017

The canonical test command used in our .travis.yml passes:

$ go test -v ./...  --run "^(integration_test)" >/dev/null; echo $?
0

But I realised that even if I make a test fail on purpose, it always passes. So I've tried running without the --run "^(integration_test)" bit, and now this hangs forever:

$ go test -v ./...   >/dev/null; echo $?

So in commit e043adf I've changed our travis file to the command above. In commit a4db27c I;'ve also made the integration tests only CONDUIT_INTEGRATION_TESTS_ENABLED, removing the need for the flag on go test.

Now it still hangs, but before hanging it shows us that another test was broken and has never been reported as such:

$ go test ./...

?   	github.com/runconduit/conduit/cli	[no test files]

--- FAIL: TestSortStatsKeys (0.00s)

	Error Trace:	stat_test.go:25

	Error:		Not equal: []string{"other/grafana", "kube-system/heapster-v1.4.3", "kube-system/kubernetes-dashboard", "kube-system/l7-default-backend", "test/backend4", "test/hello10", "test/world-deploy1", "test/world-deploy2"} (expected)

			        != []string{"kube-system/heapster-v1.4.3", "kube-system/kubernetes-dashboard", "kube-system/l7-default-backend", "other/grafana", "test/backend4", "test/hello10", "test/world-deploy1", "test/world-deploy2"} (actual)

			

			Diff:

			--- Expected

			+++ Actual

			@@ -1,3 +1,2 @@

			 ([]string) (len=8 cap=8) {

			- (string) (len=13) "other/grafana",

			  (string) (len=27) "kube-system/heapster-v1.4.3",

			@@ -5,2 +4,3 @@

			  (string) (len=30) "kube-system/l7-default-backend",

			+ (string) (len=13) "other/grafana",

			  (string) (len=13) "test/backend4",

	Messages:	Not Sorted!

		

FAIL

FAIL	github.com/runconduit/conduit/cli/cmd	0.095s

So on commit 1c5adc6 I've fixed the bug that was causing the test to hang, and now we have a broken build, as it should the case since forever. Running locally you get this:

$ go test -v ./...   >/dev/null; echo $?
1

Subsequent commits fix tests that had been failing for a while and we didn't know about because of the problems stated above. After that we're back to a good state:

$ go test -v ./...   >/dev/null; echo $?
0

@pcalcado pcalcado force-pushed the phil/fix-unit-tests branch 2 times, most recently from 32274b6 to e53d65e Compare December 19, 2017 08:08
@pcalcado pcalcado changed the title WIP Fix unit test runner and failing tests Fix unit test runner and failing tests Dec 19, 2017
@pcalcado pcalcado self-assigned this Dec 19, 2017
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Wow, good sleuthing, and thanks for cleaning this all up.

func marksParallelIfConfigured(t *testing.T) {
t.Parallel()
func checkIfIntegrationTestsAreEnabled(t *testing.T) {
integrationTestsEnvironmentVariable := "CONDUIT_INTEGRATION_TESTS_ENABLED"
Copy link
Contributor

Choose a reason for hiding this comment

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

TIOLI -- you could move this up with the rest of the consts at the top of this file.

actualBody := recorder.Body.String()

if !strings.Contains(actualBody, expectedVersionDiv) {
t.Fatalf("Expected string [%s] to be presentn in [%s]", expectedVersionDiv, actualBody)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here: presentn => present

Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Great, thanks for updating. I think this just needs a master merge, and it should be good to go.

@klingerf klingerf merged commit 683b5c0 into master Dec 19, 2017
@klingerf klingerf deleted the phil/fix-unit-tests branch December 19, 2017 22:22
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
There are many different types of things that implement `fmt::Display`
in the metrics library: lists of key-value labels, individial label keys
or values, groups of metrics output, etc. While modifying metrics code,
it can be very easy to use something that's not a label in the context
of a label.
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
Following linkerd#67 and linkerd#68, the `labels::Direction` type can be replaced with
an `impl FmtLabels for ctx::Proxy`.
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this pull request Mar 5, 2019
Following linkerd#67 and linkerd#68, the `labels::TlsStatus` type can be removed in
favor of extending underlying `ctx::transport::TlsStatus` type to
implement `FmtLabels`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants