Expose more test framework functions

[AlexNPavel]

Description of the change: This commit exposes more test framework functions, reduces the chance of namespace collision for GetNamespace, and adds godocs for all exposed test framework functions and variables.

Motivation for the change: The test framework contains various features (specifically automated cleanup of created resources and autogeneration of new testing namespaces) that may be useful outside of the context of code run via go test. This commit builds upon PR #1195, which removed the TestCtx's dependence on the testing.T field, and exposes functions that are necessary to use this outside of go test (specifically, it exposes Setup and CreateFromYAML, and adds a SetNamespace function).

[shawn-hurley]

I am trying to understand why we are doing this, is this in service of exposing scorecard tests for users?

[lilic]

Same question as Shawn had, I thought we were against exposing functions until we actually need to make them public?

[AlexNPavel]

@lilic @shawn-hurley

The main use for exposing more functions right now is going to be the new scorecard plugin that I described in the User Defined scorecard tests proposal. I feel that there are more use cases though where the automated setup and cleanup functionality of the test framework are useful. Even if we are not in a go test environment, we may want to be able to automatically clean up all resources when a function exits. This PR would allow for that.

[lilic]

What is the reason for switching to UnixNano?

[AlexNPavel]

To prevent collisions. Collisions shouldn't have happened before due to the namespace being based on the name of the test, but now it could be more likely to happen if users run create multiple contexts in parallel since those would use the same name and potentially be created in the same second. Using nanoseconds should prevent collisions.

[lilic]

Was this done initially in case we could have a nil and do we not have that case anymore?

[AlexNPavel]

I'm not sure why it was a pointer. We don't have any nil checks anywhere, so this doesn't affect anything. Just makes more sense to have it as a normal string IMO

[lilic]

Makes sense, will this be a breaking change for the users, should we explicitly mention it in the changelog.

[AlexNPavel]

I don't think it's something the users should have been accessing directly in the first place. It might make sense to unexport this field (and maybe a few others)

[camilamacedo86]

Hi @AlexNPavel,

I think that namespace can be null, is not possible the user/dev use it as an lib/imported in their project?

[joelanford]

I'm not sure if there are problematic implications, but can this function signature be refactored to something like:

type FrameworkOptions struct {
    KubeconfigPath string
    NamespacedManPath string
    Namespace string
    SingleNamespace bool
    LocalOperator bool
}

func NewFramework(opts FrameworkOptions) (*Framework, error)

Then AddToFrameworkScheme() could be a method on the Framework struct and renamed to AddToScheme().

I realize this would be a breaking change, so that's worth some discussion.

I think we should try to avoid having global mutable state as much as possible and favor returning and passing values around instead.

[AlexNPavel]

The biggest difficulty with not having the global Framework variable is how the framework would be passed around in the tests. We don't have a simple way to pass down variables through tests without global variables and having the global variable makes it easier for us to implement the main entry that sets up a single framework that all tests run after the entry can use.

[AlexNPavel]

One way to avoid the global state in pkg/test would be to have a global framework variable in the user's main_test.go file that gets set to the result of NewFramework. But this would be a pretty important breaking change that we would have to make very clear to users.

[joelanford]

Ok I think I'm understanding this a little bit better now having taken a look at https://github.com/operator-framework/operator-sdk/blob/master/test/test-framework/test/e2e/memcached_test.go.

If I understand correctly, right now it's basically required for tests to call f.MainEntry(m) to get Global setup properly. My concern is that if we open up more of the test framework and make it possible to run tests without calling f.MainEntry, then users will have to know the right order of calls to make before Global is safe to use.

If getting rid of the exported Global variable will cause too big of a breaking change, would it be possible to make its zero value (or initialized value) usable?

[openshift-ci-robot]

@AlexNPavel: PR needs rebase.

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.

[openshift-ci-robot]

@AlexNPavel: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-ansible	17f3575	link	/test e2e-aws-ansible
ci/prow/images	17f3575	link	/test images
ci/prow/e2e-aws-helm	17f3575	link	/test e2e-aws-helm
ci/prow/e2e-aws-go	17f3575	link	/test e2e-aws-go
ci/prow/e2e-aws-subcommand	17f3575	link	/test e2e-aws-subcommand

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[camilamacedo86]

Hi @AlexNPavel,

Is it a PR which still valid? If yes, could you please rebase with the master and let us know if has anything else that is missing here or just the review?