Move scaffold machinery to its own internal package

[Adirio]

Description

Scaffolding machinery is spread in the same subpackage that scaffolders. Moving it to an internal package not only helps in tidying up the code, but reduces the attack surface as the machinery can not be used from outside pkg/scaffold.

The mock version of the scaffold machinery has also received a mayor overhaul.

Motivation

This PR is scoped under #1218.

[k8s-ci-robot]

Hi @Adirio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[estroz]

/lgtm

Refactor makes a lot of sense.

[Adirio]

@estroz I rebased so lgtm label was removed.

Also, could you add ok-to-test?

[estroz]

/ok-to-test

[DirectXMan12]

/approve

rebase and get one of the reviewers to re-apply the lgtm, and this should be good

[Adirio]

@camilamacedo86 Had to rename them back to mock because golang didnt allow to have test files without test and these two are artifacts for test, but not tests themselves. Maybe we can rename them to test when we add tests. You can see the error here.

[camilamacedo86]

Hi @Adirio,

@camilamacedo86 Had to rename them back to mock because golang didnt allow to have test files without test and these two are artifacts for test, but not tests themselves. Maybe we can rename them to test when we add tests. You can see the error here.

I do not think that the error faced is because of this. See here in a personal project that I have a file called mocks_test.go which has no tests.

The problem is that the mock files are implementing funcs and not just has mock data.

# sigs.k8s.io/kubebuilder/pkg/scaffold/internal/machinery [sigs.k8s.io/kubebuilder/pkg/scaffold/internal/machinery.test]
pkg/scaffold/internal/machinery/mock_test.go:80:7: undefined: filesystem.NewMock
pkg/scaffold/internal/machinery/mock_test.go:81:4: undefined: filesystem.MockPath
pkg/scaffold/internal/machinery/mock_test.go:82:4: undefined: filesystem.MockExists
pkg/scaffold/internal/machinery/mock_test.go:83:4: undefined: filesystem.MockOutput

I have a few questions:

• Where these methods has been used?
• Why we need methods to mock the data instead of creating the mocks objects and use the "real" funcs to verify them?
• Should we really have these functions in the mock files or would make more sense call the "real" implementations with mock structures in the tests in order to verify them? How can we ensure the "real" implementations in the tests if are not use them?

[Adirio]

I do not think that the error faced is because of this. See here in a personal project that I have a file called mocks_test.go which has no tests.

The problem is that the mock files are implementing funcs and not just has mock data.

Yeah, right, the error may be because we are importing from pkg/scaffold/internal/machinery/mock_test.go some functions that are defined in pkg/scaffold/internal/filesystem/mock_test.go and that is not allowed.

I have a few questions:

• Where these methods has been used?

They are not used yet. Tests were removed in #1343 as requested in #1342 as they were doing the same as the golden test.

• Why we need methods to mock the data instead of creating the mocks objects and use the "real" funcs to verify them?
• Should we really have these functions in the mock files or would make more sense call the "real" implementations with mock structures in the tests in order to verify them? How can we ensure the "real" implementations in the tests if are not use them?

These mocks are not intended to test pkg/scaffold/internal/filesystem or pkg/scaffold/internal/machinery. These mock structures are intended to create a fake Scaffold and tests the objects that use an Scaffold like apiScaffolder in pkg/scaffold/api.go or any other Scaffolder implementation. This way, when you are testing pkg/scaffold you use a fake Scaffold from pkg/scaffold/internal/machinery so that tests are unitary, and a problem like not being allowed to write a file into disk because the OS doesn't allow you to is not traslated into the pkg/scaffold tests.

[camilamacedo86]

Hi @Adirio,

Thank you for the clarifications. Following some comments inline.

These mock structures are intended to create a fake Scaffold and tests the objects that use an Scaffold like apiScaffolder in pkg/scaffold/api.go or any other Scaffolder implementation. his way, when you are testing pkg/scaffold you use a fake Scaffold from pkg/scaffold/internal/machinery so that tests are unitary, and a problem like not being allowed to write a file into disk because the OS doesn't allow you to is not traslated into the pkg/scaffold tests.

I do not think that this should be the correct way to move forward.
We should not have func to create the mocks. We should create just the structures and call the "real" func impl in order to validate them in the tests. Is it make sense? If not, please feel free to ping me we can chat about.

[Adirio]

/test pull-kubebuilder-e2e-k8s-1-15-3

[Adirio]

Coverage:

• pkg/scaffold/internal/machinery 100%
• pkg/scaffold/internal/filesystem 79.7%
  The only statements that aren't covered by tests are related to filesystem interactions that are hard to include in unit tests.

[camilamacedo86]

Do not show strange the need to implement tests for the mock tests?
I mean, because we are not using the "real" implementations and structures and we are here re-written them we need also create tests for the tests. So, IHMO the mock files are not required at all and we should use the "real" structures and implementations in the tests by mocking the data on them.

[Adirio]

Because these are unitary tests. Unitary tests only test one thing, as oposed to end-to-end (e2e) tests, which test the whole process.

scaffold_test.go tests the machinery package. It doesn't test the underlying file system. It doesn't test if the files are created. It just tests the logic in machinery. In order to do so we need to create a fake underlying filesystem. That's our mockFileSystem.

A mock is not a test, it is a utility that other tests will use. In this case scaffold_test.go uses mockFileSystem to abstract the file system. By doing so, errors in FileSystem will not be caught by scaffold_test.go as he is not testing that package. They will be caught by filesystem_test.go.

Does a mock need to be tested? Well, they are no tests themselves so, testing mocks is never a bad thing to do. Most of the things (if not all of them) tested in mock_test.go would also be caught by scaffold_test.go, but goveralls doesn't mark as covered statements from other packages. So, despite I was claiming 79.7% was covered, the Coverage report in Travis was showing a ~35% instead. Most of the tests are really trivial, and it took 0.024s to test it, so there is no need to worry about overlapping tests.

[Adirio]

Then it won't be an unitary test. It would be an e2e test. Because if we use fileSystem we would be creating the file in the disk. We are already doing that with the golden test.

And furthermore, if we use fileSystem we would need to create functions to read the data and check it is the same.

[camilamacedo86]

Why we need the fakePlugin? Why not use the Plugin structure with mock data in the tests?

[Adirio]

Plugin is an interface. fakePlugin implements this interface. There is no Plugin structure nor data about it.

[camilamacedo86]

Why we need the fakeFile? Why not use the eFile structure with mock data in the tests?

[Adirio]

Same as Plugin, File is a interface. fakeFile implements this interface allowing us to define the data or the errors that it will yield, so that we can test the machinery package.

[camilamacedo86]

Why we do not use the funcs in pkg/scaffold/internal/filesystem/errors.go instead of re-writen them to check the errors?

[Adirio]

The functions in pkg/scaffold/internal/filesystem/errors.go check if an error is of a certain type. This functions make the mockFileSystem return an error at that step.

When you call FileSystem.Exists an error is returned (it may be nil).

• fileSysten implements the FileSystem interface. If an error happens, for example you are not allowed to read in that directory so you cna't check if it exists, fileSystem.Exists will return that error.
• mockFileSystem implements the FileSystem interface too. When you call mockFileSystem.Exists the error returned is the one you provided with MockCreateFileError.

[camilamacedo86]

Hi @Adirio,

Because these are unitary tests. Unitary tests only test one thing, as oposed to end-to-end (e2e) tests, which test the whole process.

IHMO: We do not need to test and verify other places and structures. However, we do need to create/duplicate the "real" implementation to do these mock helpers. IHMO we should use the real "structures" and methods implemented to mock the data instead of that.

However, I think we can re-check it in the future since it in POV is not a big deal at all too. Also, since @DirectXMan12 @estroz already gave their OK for this PR as well.

Following the :

/lgtm

For we are able to merge this one. 👍