Refactor tests to use vanilla testing package plus github.com/stretchr/testify

[WGH-]

Some problems with https://gopkg.in/check.v1:

• It is unmaintained. Last commit 5 years ago.
• It is not particularly popular. Most projects use vanilla testing, sometimes aided by third-party helpers such as https://github.com/stretchr/testify
• It doesn't support subtests. For example, should any of the table-driven unmarshalTests test cases fail, the entire TestUnmarshal test is immediately aborted, and you can't estimate how broken your patch is :).
• It doesn't distinguish Error (mark failed) vs. Fatal (mark failed and abort). (although in most cases you'd want to structure different checks as subtests instead, occasionally it doesn't worth it)
• It doesn't support go test -run to run tests/subtests selectively.
• go test -v doesn't show captured output.

I've had good experience with testing +github.com/stretchr/testify combo in several closed source projects.

Some examples how existing tests might change:

func TestUnmarshal(t *testing.T) {
	for _, item := range unmarshalTests {
		t.Run("", func(t *testing.T) {
			t.Logf("%q", item.data)
			value := reflect.New(reflect.ValueOf(item.value).Type())
			err := yaml.Unmarshal([]byte(item.data), value.Interface())
			if _, ok := err.(*yaml.TypeError); !ok {
				require.NoError(t, err) // fail and abort
			}
			assert.Equalf(t, item.value, value.Elem().Interface(), "error: %v", err)
		})
	}
}

/*
func (s *S) TestUnmarshal(c *C) {
	for i, item := range unmarshalTests {
		c.Logf("test %d: %q", i, item.data)
		t := reflect.ValueOf(item.value).Type()
		value := reflect.New(t)
		err := yaml.Unmarshal([]byte(item.data), value.Interface())
		if _, ok := err.(*yaml.TypeError); !ok {
			c.Assert(err, IsNil)
		}
		c.Assert(value.Elem().Interface(), DeepEquals, item.value, Commentf("error: %v", err))
	}
}
*/

Failure output example, showcasing multiple failing subtests: (yeah, diff output is somewhat excessive, but it can be useful in more complicated tests cases)

--- FAIL: TestUnmarshal (0.01s)
    --- FAIL: TestUnmarshal/#149 (0.00s)
        decode_test.go:830: "a: 2015-01-01\n"
        decode_test.go:836:
            	Error Trace:	/home/wgh/projects/_forks/go-yaml/decode_test.go:836
            	Error:      	Not equal:
            	            	expected: map[string]time.Time{"a":time.Date(2016, time.January, 1, 0, 0, 0, 0, time.UTC)}
            	            	actual  : map[string]time.Time{"a":time.Date(2015, time.January, 1, 0, 0, 0, 0, time.UTC)}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -3,3 +3,3 @@
            	            	   wall: (uint64) 0,
            	            	-  ext: (int64) 63587203200,
            	            	+  ext: (int64) 63555667200,
            	            	   loc: (*time.Location)(<nil>)
            	Test:       	TestUnmarshal/#149
            	Messages:   	error: <nil>
    --- FAIL: TestUnmarshal/#171 (0.00s)
        decode_test.go:830: "a: b\r\nc:\r\n- d\r\n- e\r\n"
        decode_test.go:836:
            	Error Trace:	/home/wgh/projects/_forks/go-yaml/decode_test.go:836
            	Error:      	Not equal:
            	            	expected: map[string]interface {}{"a":"b", "c":[]interface {}{"d", "e", "f"}}
            	            	actual  : map[string]interface {}{"a":"b", "c":[]interface {}{"d", "e"}}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -2,6 +2,5 @@
            	            	  (string) (len=1) "a": (string) (len=1) "b",
            	            	- (string) (len=1) "c": ([]interface {}) (len=3) {
            	            	+ (string) (len=1) "c": ([]interface {}) (len=2) {
            	            	   (string) (len=1) "d",
            	            	-  (string) (len=1) "e",
            	            	-  (string) (len=1) "f"
            	            	+  (string) (len=1) "e"
            	            	  }
            	Test:       	TestUnmarshal/#171
            	Messages:   	error: <nil>

I'm willing to undertake this refactoring, if maintainers agree with me.

[dolmen]

Please do not depend on Testify packages.

Testify depends on gopkg.in/yaml.v3 because its assert package has a YAMLEq function.

At some point Testify may switch to this fork. But that would create a circular module dependency, and that pollutes go.sum of all downstream projects.

Note: I'm a Testify co-maintainer.
Cc: @brackendawson

[WGH-]

Heh, that was unexpected. Thanks for noticing the problem.

Perhaps, dependency on gopkg.in/yaml.v3 in main testify module was a mistake? And what was actually needed was easy Equal customization?

[dolmen]

YAMLEq was definitely a mistake because of the dependency it brought. I wasn't involved at the time.

I have added build tags to allow end users to have the choice of unlinking the YAML module, or to link another one. See stretchr/testify#1579. But we can't just remove the dependency as that would be a breaking change.

[brackendawson]

Hi, another co-maintainer of testify here. While testify is maintained, and I think it will be for the foreseeable future. It may not be the best choice where you don't already depend on it.

The aim for its maintenance is to ensure large projects who use it extensively don't have to re-write their tests, and to a lesser extent so that developers who are used to it don't have to learn a new tool.

However testify has a lot of mistakes in its API, if you do use it we recommend using the linter to avoid errors arising from these. But there are safer assertion libraries out there.

It's very likely that testify will need to switch to this fork, which would make a cyclic module dependency which we'd have to fix like this: https://github.com/stretchr/testify/blob/7127b6099902025b069e842d93bcdd6e8462eb5c/go.mod#L14-L16

Personally, I would prefer that this repo only use the standard library for tests because it is a library which is expected to be imported by many other modules and it would be better not to impose a transient dependency on them just because of your tests. I used to hold the github.com/go-sql-driver/mysql driver in high regard for having zero dependencies, but they added one. 😆

[WGH-]

Personally, I would prefer that this repo only use the standard library for tests because it is a library which is expected to be imported by many other modules and it would be better not to impose a transient dependency on them just because of your tests

It only leads to some go.sum pollution, though. Test dependencies are not added to go.mod thanks to module graph pruning.

[WGH-]

However testify has a lot of mistakes in its API, if you do use it we recommend using the linter to avoid errors arising from these. But there are safer assertion libraries out there.

@brackendawson, what other libraries do you recommend?

[brackendawson]

For a library such as this I recommend the standard library. For a program I would recommand https://pkg.go.dev/github.com/matryer/is and https://pkg.go.dev/github.com/golang/mock.

[carloslima]

I've opened #52 to remove gopkg.in/check.v1 in favor of plain standard testing library. Please take a look.

[ccoVeille]

@ingydotnet This is not really closed, only gopkg.in/check.v1 was replaced, testify is still there and need to be replaced by vanilla testing.

The issue comes from the closes #21 that was an error.

Feel free to update the issue title and create a new one, or simply reopen this issue

[WGH-]

This is not really closed, only gopkg.in/check.v1 was replaced, testify is still there and need to be replaced by vanilla testing.

On the contrary, gopkg.in/check.v1 was dropped and testify was not introduced. It's 100% vanilla now.

My original suggestion when I opened the issue was to add testify, which turned out to be not that simple due to circular dependency.

Personally, I'm fine with just gopkg.in/check.v1 removed.

[ccoVeille]

Oh. I misunderstood sorry, great then. I'm sorry for the noise