fix: tests failure, testifylint issues, pin golangci-lint version

[alexandear]

The PR fixes failing tests and explicitly sets the golangci-lint's version to prevent new lint issues from being introduced during an upgrade.

Also, the PR correctly fixes testifylint.error-nil issues (that were mistakenly added in the #3102):

• require.Nil -> require.NoError.
• require.NotNil -> require.Error.
• require.Nil -> require.Equal(t, (*gqlerror.Error)(nil), err) for AddUpload and .MutateOperationParameters (see explanation below).

Explanation

The functions AddUpload and MutateOperationParameters returns *gqlerror.Error instead of error:

func (p *RawParams) AddUpload(upload Upload, key, path string) *gqlerror.Error

MutateOperationParameters(ctx context.Context, rawParams *graphql.RawParams) *gqlerror.Error

In the event of no error, these functions return nil, which transforms to nil of the type *gqlerror.Error. That's why we need to assert with require.Equal(t, (*gqlerror.Error)(nil), err).

The proper fix would involve changing the return parameter from *gqlerror.Error to error. However, this could break backward compatibility.

References

• https://yourbasic.org/golang/gotcha-why-nil-error-not-equal-nil/
• https://stackoverflow.com/questions/53892508/golang-returning-nil-does-not-return-nil

[StevenACoffman]

Thanks!

In the event of no error, these functions return nil, which transforms to nil of the type *gqlerror.Error. That's why we need to assert with require.Equal(t, (*gqlerror.Error)(nil), err).

The proper fix would involve changing the return parameter from *gqlerror.Error to error. However, this could break backward compatibility.

This is intentional, as the *gqlerror.Error type contains more helpful information than a stdlib error but both conform to the error interface { Error() string }, so I think this is appropriate. We could make a helper to make it a little more readable.

[alexandear]

This is intentional, as the *gqlerror.Error type contains more helpful information than a stdlib error but both conform to the error interface { Error() string }, so I think this is appropriate. We could make a helper to make it a little more readable.

Idiomatic Go tells, that the function from a library should usually return an error in any case. Hoverer, callers can use a type switch or a type assertion to look for specific errors and extract details. See https://go.dev/doc/effective_go#errors

In our case, changing AddUpload to

func (p *RawParams) AddUpload(upload Upload, key, path string) error

would lead to the following caller's code:

if err := params.AddUpload(upload, key, path); err != nil {
	gErr, ok := err.(*gqlerror.Error)
	if ok {
		// do something with gErr
	}