Skip to content

Add core of plugin test framework Stepwise #9166

Merged
catsby merged 33 commits into
vlt091-plugin-testingfrom
stepwise-core
Jun 18, 2020
Merged

Add core of plugin test framework Stepwise #9166
catsby merged 33 commits into
vlt091-plugin-testingfrom
stepwise-core

Conversation

@catsby
Copy link
Copy Markdown
Contributor

@catsby catsby commented Jun 8, 2020

This is the start of implementing a new plugin testing framework codenamed stepwise. The goal is to implement a testing framework comparable to (and to eventually replace) the existing "logicaltest" framework found in helper/testhelpers/logical/testing.go, however using an actual, external, running instance of Vault. The canonical and first coming example would be using Docker to run Vault and run plugin tests against it.

This PR introduces the basic types and interface to establish this framework, and the Run method that executes the user defined steps, and some simple tests. This PR is currently in DRAFT form for early review and feedback.

Update 2020-06-12: added some tests, removing "draft"

@catsby catsby added this to the 1.5 milestone Jun 8, 2020
@catsby catsby requested a review from pcman312 June 8, 2020 14:48
Copy link
Copy Markdown

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left few comments/suggestions inline, but I'm curious: Why do we want to have own testing framework with own types (Case, Steps, etc)? I think I see several issues the framework is going to solve, but I also fear that:

  • It is going to be very limiting. For example, some plugins might need to perform actions on external systems between steps, etc.
  • Because of the above, it will be tempting to add more features into the framework and it will increase maintanance costs. The fact that the package is going to be used externally adds more flavour into this.
  • It is going to be a quite big piece for a very specific purpose which plugin authors will need to learn.

I'm wondering if an alternative approach where we are not trying to wrap Go's testing package with abstractions would be a better fit? I (vaguely at this stage) see it as collection of functions you can use inside of the Go's standard test. Something similar to gomock's approach where you call controller functions inside of a test:

func TestFoo(t *testing.T) {
    ctrl := gomock.NewController(t)
    defer ctrl.Finish()

    m := NewMockFoo(ctrl)

    // Do something
}

Apologies if I am bothering you with stupid suggestions. I'm not super familiar with the project and probably missing some background for this PR.

Comment thread sdk/testing/stepwise/stepwise.go Outdated
Comment thread sdk/testing/stepwise/stepwise.go Outdated
Comment thread sdk/testing/stepwise/stepwise.go Outdated
Comment thread sdk/testing/stepwise/stepwise.go
Comment thread sdk/testing/stepwise/stepwise.go Outdated
@catsby
Copy link
Copy Markdown
Contributor Author

catsby commented Jun 12, 2020

Hey @m1kola thanks for the review! I'll reply to your items in-line below.

Why do we want to have own testing framework with own types (Case, Steps, etc)?

Vault already has an existing testing framework that's the predecessor to this work in helper/testhelpers/testing.go:

The primary goal(s) of this work here is to effectively overhaul or reintroduce it but utilize an actual instance of Vault instead of some of our Vault core test functions, and to avoid calling plugin methods directly through the plugin's HandleRequest methods. As for using our own types, we're loosely following the pattern set by Terraform Provider acceptance testing, which has served us (HashiCorp at large) very well over the years.

* It is going to be very limiting. For example, some plugins might need to perform actions on external systems between steps, etc.

It may be limiting at times or in certain areas, yes. The framework is not envisioned as an end-all for plugin testing, but instead another tool available, and one that comes with the Vault SDK. We felt that a plugin testing framework that manages an external Vault instances, other external infrastructure, and the required coordination between them may be better suited to be a separate external tool/framework. While this framework does some of that and has limitations, we hope it to be a compromise. Hopefully you can agree!

* Because of the above, it will be tempting to add more features into the framework and it will increase maintanance costs. The fact that the package is going to be used externally adds more flavour into this.

The intent / design is meant for a specific use-case or style of tests, and as a result will likely not be the most robust framework possible. I'm hopeful we can keep the framework focused on that use-case and resist the temptation to add too much and increase the maintenance costs.

* It is going to be a quite big piece for a very specific purpose which plugin authors will need to learn.

Plugin authors don’t have to learn this if they don’t choose to, but I agree that if they do there will be a learning curve. We feel this specific purpose is much needed in Vault and the Vault ecosystem and we certainly hope people will find it worthwhile to learn.

I'm wondering if an alternative approach where we are not trying to wrap Go's testing package with abstractions would be a better fit? I (vaguely at this stage) see it as collection of functions you can use inside of the Go's standard test.

As I mentioned above, the framework has a specific use-case and so we're explicitly trying to be prescriptive in how to write or construct these tests.

Something similar to gomock's approach where you call controller functions inside of a test:

Unfortunately I haven't yet invested the time to learn how to use golang/mock, so I'm not sure what your example provides. Could you elaborate and educate me here? If it fits within the paradigm I've described above I'll certainly investigate more.

Apologies if I am bothering you with stupid suggestions. I'm not super familiar with the project and probably missing some background for this PR.

No apologies needed! The project was discussed and planned internally and unfortunately not shared with the community until this first PR. Hopefully it will make more sense as follow-up PRs land.

Thanks again for the feedback and review!

Cheers,
Clint

@catsby catsby changed the title Draft: Add core of plugin test framework Stepwise Add core of plugin test framework Stepwise Jun 12, 2020
@catsby catsby marked this pull request as ready for review June 12, 2020 20:47
Copy link
Copy Markdown
Contributor

@pcman312 pcman312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall! Just some questions, suggestions for improvements, and some nits.

Comment thread sdk/testing/stepwise/helpers.go Outdated
Comment thread sdk/testing/stepwise/helpers.go Outdated
Comment thread sdk/testing/stepwise/helpers.go Outdated
Comment thread sdk/testing/stepwise/helpers.go Outdated
Comment thread sdk/testing/stepwise/helpers.go Outdated
Comment thread sdk/testing/stepwise/stepwise.go
Comment thread sdk/testing/stepwise/stepwise.go Outdated
Comment thread sdk/testing/stepwise/stepwise_test.go Outdated
Comment thread sdk/testing/stepwise/stepwise_test.go
Comment thread sdk/testing/stepwise/stepwise_test.go
catsby and others added 2 commits June 15, 2020 16:29
Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Comment thread sdk/testing/stepwise/stepwise.go Outdated
@m1kola
Copy link
Copy Markdown

m1kola commented Jun 16, 2020

@catsby thanks for the clarifications. This is very helpful in learning more background for this PR and Vault in general.

I understand that the idea is to provide a framework for a very specific use-case of tests, but I'm not convinced that there is a need to enforce such a strict structure (defining a Case, Steps for each test, etc).

As for using our own types, we're loosely following the pattern set by Terraform Provider acceptance testing, which has served us (HashiCorp at large) very well over the years.

Thanks for this reference. I had a quick look at the testing framework from terraform-plugin-sdk: I think the number of configuration options the terraform plugin testing framework provides and amount of commits it receives supports my poin about adding complexity: it is going to be hard to resist the temptation to add too much :) I think we should give as little options as possible and make the framework as minimal as possible. Give more power and responsibilities to the plugin authors.

I'm looking at TestBackend_basic_docker and keeping in mind other cases... I think I'll be able to come up with a design which will have the following characteristics (among others):

  • Gives a way to setup/teardown an environment
  • Exports fewer types and options
    • Ideally the core of framework only exports something like the StepwiseEnvironment interface with Setup/Teardown, Client and (maybe) few other functions.
      • But it should be ok to add and export implementations of the interfaces (eg. docker environment) to the SDK as a subpackage.
      • Shoukd be easy to add own implementation of the interfaces
    • Less types & options should make it easier to maintain it as part of the SDK
    • Should also make it easier to learn (looking at 10s of options of terraform-plugin-sdk again and it's not easy to digest)
  • Does not impose a test structure
    • Plugin authors will be using the testing framework from Go standard library more direclty. It is a tool familiar to many Go developers (easier adoption)
    • Our testing SDK will work in a similar way to gomock's Controller: plugin authors will be calling functions from the SDK as part of standard Go test (I know you are not familiar with gomock, will try to provide a better example ASAP: see suggestion below)
    • Gives plugin authors more control & power (can change state of the external system, for example)
    • But makes plugin authors to be responsible for a bit more. For example, they will be explicitly calling the client and will have to do assertions such as ExpectError.
  • (Maybe) Provides utilities (such as CompilePlugin).
  • Will not be compatible with helper/testhelpers/testing.go (not sure if it's a goal as part of current Stepwise efforts)
  • Will not be feel familiar to people who already used terraform-plugin-sdk or helper/testhelpers/testing.go (again, not sure if it's important)

@catsby @pcman312 what do you think? Is it worth exploring? If yes, as a starting point I can rewrite TestBackend_basic_docker to use the design I have in mind (roughly, without implementation of the framework). It will demonstrate us how it is going to look from plugin author's perspective. After this I can write more detailed design document or jump straight to the implementation (going to re-use a lot of things from this PR).

catsby and others added 7 commits June 16, 2020 14:06
return `nil` and not `err` at the end

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
@catsby
Copy link
Copy Markdown
Contributor Author

catsby commented Jun 18, 2020

Hey @m1kola -

Thank you again for your continued feedback!

Regarding adding complexity, we certainly don’t consider the framework “done” as of this PR, with more features, changes, and improvements planned in the near future. The focus will remain the same though: providing a prescriptive way to test a plugin through invoking the Vault API itself. I would agree with your sentiments on empowering plugin authors here if we intended or even envisioned stepwise to be the end-all solution for testing plugins, however that is not the case. The stepwise framework is meant to be a structured method of testing plugins in a specific way, effectively interacting with the plugin through Vault as a user would. We realize that a testing framework specific to the project it is included in will be unfamiliar for many Go developers, which is why stepwise should not be the only way to test plugins. The intent is to provide a structured way of opaque testing of plugins, with as little mocking as possible (preferably zero mocking).

I don’t mean to discourage your efforts to add or improve Vault or Vault plugin testing, I hope you don’t receive this reply in that way. There currently exists several internal testing functions and methods for wiring a plugin up with a Vault core (often a simulated one) such as TestCore or NewTestCluster in vault/testing.go. Alternatively you can often bypass having a core at all and directly call backend methods with minimal “Vault” included, by invoking the HandleRequest method on the framework.Backend type (some examples in the Transit backend here). These methods and testing styles meant to give developers flexibility in how they structure and test plugins, and are absolutely a valuable tool that I’m sure would benefit from many of the additions you mention.

I want to reiterate that I don’t mean to discourage you from pursuing the ideas you have for improving plugin testing for Vault, quite the opposite! I would love to see and hear more on your ideas, but I want to be clear that they run contrary to the goals of the stepwise framework as it stands today. With time and reflection, perhaps some of those ideas will come to be included, if appropriate.

Thank you again for your input here! I hope this reply answers your questions, at least for now.

Cheers,
Clint

@catsby catsby merged commit 879fa5e into vlt091-plugin-testing Jun 18, 2020
catsby added a commit that referenced this pull request Jun 23, 2020
* Resolve merge conflicts and updates from running a test

* move testing/_test.go over to legacy

* updates

* Add core of plugin test framework Stepwise  (#9166)

* adding stepwise testing, but there are protocol buff error :/

* move file and update sdk/go.mo

* update/sync modules

* update from other branch

* update sdk/go.mod

* some cleanups after feedback

* remove enviornments from this PR

* update vendor

* change from running go mod tidy

* change from go mod tidy

* Update sdk/testing/stepwise/helpers.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* Update sdk/testing/stepwise/helpers.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* change panic to error

* Update sdk/testing/stepwise/helpers.go

return `nil` and not `err` at the end

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* Defer close() on successful Open of a file

* document the re-creation of steps

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* remove unused BarrierKeys()

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* updates from feedback

* fix return with bad arguments

* Rename things:

- StepOperation -> Operation
- StepwiseEnvironment -> Environment
- StepCheckFunc -> AssertionFunc
- step.Check -> step.Assert

* document the environment interface methods

* rename EnvironmentOptions to MountOptions

* rename Name to RegistryName

* remove ExpectError because it's redundant

* minor doc update

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* add checkShouldRun function

* remove redundant return

* remove vestigial PreCheck function

* add tt.Helper() to makeRequest

* minor code formatting and document 1-based index for log output of Steps

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* minor updates

* update sdk

* use local reference for api, vault dep

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Calvin Leung Huang <cleung2010@gmail.com>

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Calvin Leung Huang <cleung2010@gmail.com>

* cleanup some defer functions

* call fatal if environment setup fails, and don't call teardown

* defer re-setting client token in makeRequest

* Move legacy logicaltest back to testhelpers

* update mods and test files with go mod tidy

* go mod vendor

* remove relative replace directives

* restore old logical test location

* move declaration to main stepwise file

* remove index var and use i+1

* add testing for write, delete paths of makeRequest

* update stepwise core testing to do request counting

* remove unused methods

* Update sdk/testing/stepwise/stepwise.go

remove dead line

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

* Update sdk/testing/stepwise/stepwise.go

fix capitalization in code comment

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

* update code comments for SkipTeardown to clarify its use

* update stepwise

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Co-authored-by: Calvin Leung Huang <cleung2010@gmail.com>
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
andaley pushed a commit that referenced this pull request Jul 17, 2020
* Resolve merge conflicts and updates from running a test

* move testing/_test.go over to legacy

* updates

* Add core of plugin test framework Stepwise  (#9166)

* adding stepwise testing, but there are protocol buff error :/

* move file and update sdk/go.mo

* update/sync modules

* update from other branch

* update sdk/go.mod

* some cleanups after feedback

* remove enviornments from this PR

* update vendor

* change from running go mod tidy

* change from go mod tidy

* Update sdk/testing/stepwise/helpers.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* Update sdk/testing/stepwise/helpers.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* change panic to error

* Update sdk/testing/stepwise/helpers.go

return `nil` and not `err` at the end

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* Defer close() on successful Open of a file

* document the re-creation of steps

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* remove unused BarrierKeys()

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* updates from feedback

* fix return with bad arguments

* Rename things:

- StepOperation -> Operation
- StepwiseEnvironment -> Environment
- StepCheckFunc -> AssertionFunc
- step.Check -> step.Assert

* document the environment interface methods

* rename EnvironmentOptions to MountOptions

* rename Name to RegistryName

* remove ExpectError because it's redundant

* minor doc update

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* add checkShouldRun function

* remove redundant return

* remove vestigial PreCheck function

* add tt.Helper() to makeRequest

* minor code formatting and document 1-based index for log output of Steps

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* minor updates

* update sdk

* use local reference for api, vault dep

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Calvin Leung Huang <cleung2010@gmail.com>

* Update sdk/testing/stepwise/stepwise.go

Co-authored-by: Calvin Leung Huang <cleung2010@gmail.com>

* cleanup some defer functions

* call fatal if environment setup fails, and don't call teardown

* defer re-setting client token in makeRequest

* Move legacy logicaltest back to testhelpers

* update mods and test files with go mod tidy

* go mod vendor

* remove relative replace directives

* restore old logical test location

* move declaration to main stepwise file

* remove index var and use i+1

* add testing for write, delete paths of makeRequest

* update stepwise core testing to do request counting

* remove unused methods

* Update sdk/testing/stepwise/stepwise.go

remove dead line

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

* Update sdk/testing/stepwise/stepwise.go

fix capitalization in code comment

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

* update code comments for SkipTeardown to clarify its use

* update stepwise

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Co-authored-by: Calvin Leung Huang <cleung2010@gmail.com>
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants