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

Already on GitHub? Sign in to your account

add new data-driven test suite + move all existing tests to it #178

Merged
merged 16 commits into from Dec 10, 2017

Conversation

slimsag
Copy link
Member

@slimsag slimsag commented Dec 4, 2017

This change moves us over to a data-driven test suite and moves all existing tests over to it.

The basic idea is that instead of writing a bunch of DOM mocking code like we used to, we now just create a test by performing some Vecty actions and the DOM operations are recorded to a file. We keep the file of wanted DOM operations around, and they are compared.

For example, TestSetTitle used to look like this:

func TestSetTitle(t *testing.T) {
	titleSet := ""
	document := &mockObject{
		set: func(key string, value interface{}) {
			if key != "title" {
				panic(fmt.Sprintf(`expected document.set "title", not %q`, key))
			}
			titleSet = value.(string)
		},
	}
	global = &mockObject{
		get: map[string]jsObject{
			"document": document,
		},
	}
	want := "foobar"
	SetTitle(want)
	if titleSet != want {
		t.Fatalf("titleSet is %q, want %q", titleSet, want)
	}
}

The vast majority of this code is setting up a fake JS DOM global, global.get, document, document.set, etc. objects & methods in order to properly check was global.document.title set to the string "foobar"?. It's far too verbose and unreadable for such a simple test. The problem is even worse for larger tests that we have.

Instead of the above, the new test suite makes this test be written just as:

// TestSetTitle tests that the SetTitle function performs the correct DOM
// operations.
func TestSetTitle(t *testing.T) {
	ts := testSuite(t, "TestSetTitle")
	defer ts.done()

	SetTitle("foobartitle")
}

When go test is ran, a new file testdata/TestSetTitle.got.txt is written which looks like this:

global.Get("document")
global.Get("document").Set("title", "foobartitle")

It tells us that first global.Get("document") was called, and subsequently Set("title", "foobartitle") was called on the pointer returned by global.Get("document") originally (that is, the second line does not say that global.Get("document") was called twice).

The test suite also tells you that testdata/TestSetTitle.got.txt does not match (the non-existent) testdata/TestSetTitle.want.txt file and it shows you a nice colored diff and informs you:

    	testsuite_test.go:211: to accept these changes:
    		
    		$ mv testdata/TestSetTitle.got.txt testdata/TestSetTitle.want.txt

If you then run the suggested command and commit the testdata folder changes, the test now succeeds & no regressions can occur here in the future.

Additionally, what the diff stats on GitHub here does not show is that:

  1. 1,668 lines of hand-written Go test code have been removed(!)
  2. Only 588 lines of Go code have been added to build this new test suite.
  3. 493 lines of automatically generated testdata/**.want.txt files have been added.

I think that this new test suite will substantially help in our goal to achieve 100% test coverage (#168) because adding a new test requires no more mocking. Just perform some Vecty actions in the Go test code, run go test from the command line, confirm the DOM operations look correct & follow the instructions when prompted. 馃槂

Note: I will merge this change not as a squash commit, because it is in logical commits & is useful to see how each individual test was migrated over to the new test suite.

Helps #168

/cc @pdf (and @shurcooL who may find this to be an interesting/cool simplification :) )

@ghost
Copy link

ghost commented Dec 7, 2017

This is awesome.

@slimsag
Copy link
Member Author

slimsag commented Dec 10, 2017

This change looks generally well received (two hearts + 1 good comment), and only affects our test suite so I will merge it now. Plus, it's blocking some other changes I have in the pipeline like #180.

Also, I've decided I will not squash merge this change both because it is disabled in the repo (to prevent myself from doing that) and to stay consistent.

@slimsag slimsag merged commit 7513043 into master Dec 10, 2017
@slimsag slimsag deleted the sg/improved-testsuite branch December 10, 2017 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant