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

Remove "testing" import from public interface #22

Merged
merged 2 commits into from
Aug 25, 2019
Merged

Remove "testing" import from public interface #22

merged 2 commits into from
Aug 25, 2019

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Aug 24, 2019

Currently, the DoTestCase, DoTestCaseFile, and DoTestCases functions,
are exposed as part of the public interface.

func DoTestCase(m Markdown, testCase MarkdownTestCase, t *testing.T)
func DoTestCaseFile(m Markdown, filename string, t *testing.T)
func DoTestCases(m goldmark.Markdown, cases []MarkdownTestCase, t *testing.T)

Implementing these functions requires importing the testing package.
Importing the testing package automatically registers a number of
global command line flags.

The effect of this is that any application using the standard flag
package with goldmark will automatically get a number of unwanted
command line flags. This is verifiable with the following program,

package main

import (
	"flag"

	_ "github.com/yuin/goldmark"
)

func main() {
	flag.Parse()
}

To fix this, the testing import needs to be removed from all non-test
files. There are two ways to go about it,

  • If the functions are meant for external use, you can define an
    interface with a subset of the methods of testing.T and switch the
    functions to consume that. This is what testing libraries like
    gomock and testify do.
  • If the functions are meant for internal use, you can remove them from
    the public interface of the library and use them only from tests.

Since these functions are meant to be used by external extensions, I've
introduced a TestingT interface that is a subset of the functionality
provided by testing.T. It supports the standard operations: logging,
skiping, and failing tests,

This also moves those functions into a testutil subpackage. This will help
keep the top-level goldmark package clean and limited to core functionality.

(Note that tests in the top-level goldmark package that make use of
these functions must now use the package name goldmark_test so that
they're considered separate from the main goldmark package, otherwise
you'll see an import cycle: goldmark imports testutil imports goldmark.)

@yuin
Copy link
Owner

yuin commented Aug 25, 2019

Nice PR.

Hmm, these functions will be used by external extensions, so these functions are not 'internal` functions.

So testutil may be good for just /testutil than /internal/testutil .
Or It is better to take different way like you've suggested?

@abhinav
Copy link
Contributor Author

abhinav commented Aug 25, 2019

Ah, so these are meant to be used externally. I'll move them to /testutil
and add a TestingT interface. That keeps testing APIs out of the main API
and still makes it possible to use the library without importing "testing".
Plus if the testing APIs ever get complicated and need their own tests, a
TestingT interface can have a fake implementation.

Currently, the DoTestCase, DoTestCaseFile, and DoTestCases functions,
are exposed as part of the public interface.

    func DoTestCase(m Markdown, testCase MarkdownTestCase, t *testing.T)
    func DoTestCaseFile(m Markdown, filename string, t *testing.T)
    func DoTestCases(m goldmark.Markdown, cases []MarkdownTestCase, t *testing.T)

Implementing these functions requires importing the `testing` package.
Importing the `testing` package [automatically registers][1] a number of
global [command line flags][2].

  [1]: https://golang.org/src/testing/testing.go#L252
  [2]: https://golang.org/cmd/go/#hdr-Testing_flags

The effect of this is that any application using the standard `flag`
package with goldmark will automatically get a number of unwanted
command line flags. This is verifiable with the following program,

    package main

    import (
    	"flag"

    	_ "github.com/yuin/goldmark"
    )

    func main() {
    	flag.Parse()
    }

To fix this, the `testing` import needs to be removed from all non-test
files. There are two ways to go about it,

- If the functions are meant for external use, you can define an
  interface with a subset of the methods of `testing.T` and switch the
  functions to consume that. This is what testing libraries like
  [gomock] and [testify] do.
- If the functions are meant for internal use, you can remove them from
  the public interface of the library and use them only from tests.

  [gomock]: https://godoc.org/github.com/golang/mock/gomock#TestReporter
  [testify]: https://godoc.org/github.com/stretchr/testify/require#TestingT

Since these functions are meant to be used by external extensions, I've
introduced a TestingT interface that is a subset of the functionality
provided by `testing.T`. It supports the standard operations: logging,
skiping, and failing tests,
This moves the following functions meant for use from tests into a
testutil subpackage.

    func DoTestCase(m Markdown, testCase MarkdownTestCase, t TestingT)
    func DoTestCaseFile(m Markdown, filename string, t TestingT)
    func DoTestCases(m goldmark.Markdown, cases []MarkdownTestCase, t TestingT)

This will help keep the top-level goldmark package clean and limited to
core functionality.

(Note that tests in the top-level goldmark package that make use of
these functions must now use the package name `goldmark_test` so that
they're considered separate from the main `goldmark` package, otherwise
you'll see an import cycle: goldmark imports testutil imports goldmark.)
@yuin
Copy link
Owner

yuin commented Aug 25, 2019

LGTM, Thanks!

@yuin yuin merged commit 4a77068 into yuin:master Aug 25, 2019
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

2 participants