Support parallel testing in terraform testing framework #16807

metacpp · 2017-12-01T05:10:36Z

Add Parallel() in the TestT interface so that testing.T.Parallel can be invoked in each test function.
Introduced the TF_PARA for user to enable it while running "go test".
Add unit test case.

The purpose for this PR is to enable the parallel testing for all go test cases without changing any line of legacy code base. It requires that your test cases are all independent before turn it on through environment variable. If you just want to enable it for specified cases, please call t.Parallel() in your testing function one by one.

1. Add Parallel() in the TestT interface so that testing.T.Parallel can be invoked in each test function. 2. Introduced the TF_PARA for user to enable it while running "go test". 3. Add unit test case.

vancluever · 2017-12-01T19:15:42Z

Hey @metacpp - this is a great idea and I think it's sorely needed on some tests!

One thing I'm thinking - can we move the flag to a bool on TestCase instead? There will definitely be some tests in a provider where parallelism will not be possible period, and we want to be able to run an entire provider's test suite in one run - this will not be possible with TF_PARA set on say something like make testacc. I'll annotate the line I'm thinking the logic could be changed on, and it will allow for the granularity in a provider where some tests run in parallel but some don't.

PS: This would be non-invasive as well - as if the flag is a zero value you won't need to declare it in TestCase.

vancluever · 2017-12-01T19:16:56Z

helper/resource/testing.go

+	// will be run parallel.
+	// example usage:
+	//	TF_PARA=1 go test [TEST] [TESTARGS] -parallel=n
+	if os.Getenv(ParaTestEnvVar) != "" {


Hey @metacpp - this is where I think the logic could be changed if it was a flag in TestCase:

if c.Parallel { t.Parallel() }

@vancluever thanks for comments.

I was thinking about the possibility to allow each different acc tests to decide if it's parallel or not. For new test case to be added, this seems to be pretty good way. But for provider like azurerm, we have 618+ cases, all of them are qualified parallel cases. If we choose to enable the parallel run at test case level, we will need to change 618+ test cases in our code repo to set "Parallel" to be true. Even we use some text processing tool/script to do the work in a batch, @tombuildsstuff might have concern on the big change list and potential conflicts to be resolved if the test files are frequently changed, but we should be able to find out a way to resolve it.

I do have another suggestion here: instead of having "Parallel" as field, can we have "SkipParallel" instead ? The new code logic will be like:

if os.Getenv(ParaTestEnvVar) != "" && !SkipParallel {
t.Parallel()
}

What do you think ?

Another solution is for each different provider, you can have a thin layer testing framework built on top of terra testing, which will have some default settings(ut, parallel) for its own scenario. But it will still need to change all the legacy test files in that provider repo, I am not sure if @tombuildsstuff will agree so many changes.

Hey @metacpp, will reply with most of the content out-line, but just wanted to mention that I don''t think

if os.Getenv(ParaTestEnvVar) != "" && !SkipParallel { t.Parallel() }

Is a good approach too as this introduces 2 flags, rather than just one.

Will reply with the rest of the info in the main thread.

@vancluever I did have a similar PR on azurerm provider, and I was suggested by @tombuildsstuff to do the PR in Terraform runtime coz we thought that this change might benefit more people who are using resource.Test in their repo. I am fine with introducing just one flag here, aka, IsParallel, just like IsUnitTest. And to save changes in each provider repo, we can discuss in below thread.

vancluever · 2017-12-02T18:46:30Z

Hey @metacpp, I'm replying up here as I think the content of this reply makes sense out-line.

After giving this a bit more thought and reviewing the options, I don't necessarily think that this is a good fit for the testing framework right now. Right now resource.Test doesn't really concern itself with interfacing too much with testing.T, and it might be best to keep it this way.

What I would suggest instead, and what would probably be the most non-invasive compromise, would be to add a hook into the AzureRM provider's normal framework that's offered by PreCheck that selectively or globally enables parallel testing for the entire provider (example: adding this into your testAccPreCheck function).

This would get you what you want without any modifications here or forcing any interface onto other providers, which would allow them to make a decision to adopt parallel testing at their own convenience and in a fashion that makes sense for them.

As an example in another provider: the vSphere provider uses sub-tests, so we can call t.Parallel within t.Run to flag parallel testing for an entire resource's suite without affecting other resources. As some of our tests are self-hosting and create the necessary dependencies themselves, there may be naming conflicts between suites and the tradeoff between parallelism and refactoring might not be worth it, or just flat out impossible. In any case, within the provider we are free to call t.Parallel where ever it makes sense to do so, without having to work with or work around the resource testing framework.

Going to defer to @apparentlymart and @jbardin on this one, but I think this is the best thing to do until it's evident that parallelism is explicitly needed in the framework.

Thanks!

metacpp · 2017-12-04T04:15:27Z

@vancluever It's very hard to say that adding Parallelism at resource.Test level is necessary coz each different provider will have its own testing strategy and scenario, that's why I choose to limit the scope to azurerm provider only in my another PR in azurerm repo.

Let me know if "adding IsParallel to C" is something align with strategy and roadmap of terraform's testing framework. If not, I am fine that we limit the changes into azurerm provider only. We have been stuck by low performance of acc testing for quite a long time, and we really want to resolve in a more generic way.

metacpp · 2017-12-04T05:15:14Z

@vancluever the code is updated, please let me know if you have any other question.

apparentlymart · 2017-12-04T19:27:11Z

Thanks for working on this, @metacpp, and thanks for all the analysis so far, @vancluever.

I agree that it would be good to solve this at the provider level if it can be solved that way, for the reasons you noted.

Given how many different callers we have for the testing framework, and thus the potential risk/pain of making changes here (e.g. if we find that this approach doesn't generalize as well as expected), I think it would be helpful to try out provider-specific solutions in a few different providers that have different needs, and then based on that we can see if there is a generalization that makes sense across all (or some) of the use-cases.

metacpp · 2017-12-05T06:01:44Z

@apparentlymart can you take a look at my latest PR, it's just adding parallelism at test case level and it might benefit the people who want to have the test case being run in parallel with simple bool flag.

bflad · 2018-09-13T15:53:35Z

Hi @metacpp! 👋 Thanks for submitting this and apologies this has been a longstanding pull request.

We recently merged #18688, which approaches this problem slightly differently by making the behavior opt-in via the function name (ParallelTest()) instead of requiring additions to individual TestCase or provider-wide in any sense. Hopefully this allows for an iterative transition path for many of the providers if their acceptance testing is properly prepared (e.g. acceptance tests properly randomize naming, etc.) by a simple function name refactor with one less line of code required per acceptance test.

The new functionality is now available in master and will release in the next tagged release, which looks like it will be Terraform 0.11.9.

Thank you again for bringing up this discussion and helping make Terraform better! 🎉

metacpp · 2018-09-17T00:15:55Z

@bflad thanks for the updates. Do you know that when 0.11.9 will be released?

paultyng · 2018-09-17T12:51:48Z

@metacpp I don't think its necessary to wait for the release to use the provider SDK. Since its on master, feel free to use this commit hash as a dependency.

If/when we move away from govendor though that will probably change.

ghost · 2020-04-02T01:59:36Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

metacpp added 4 commits November 30, 2017 18:11

Support parallel testing in terraform testing framework:

124be56

1. Add Parallel() in the TestT interface so that testing.T.Parallel can be invoked in each test function. 2. Introduced the TF_PARA for user to enable it while running "go test". 3. Add unit test case.

add missed parallel method in mockT.

9d3a6bf

Update the unit test case for ParaTestEnv.

c159bec

Fix the styling issue for mockT.

3dcda5a

tombuildsstuff requested review from apparentlymart and jbardin December 1, 2017 11:52

tombuildsstuff self-assigned this Dec 1, 2017

tombuildsstuff added the enhancement label Dec 1, 2017

tombuildsstuff requested a review from vancluever December 1, 2017 18:59

vancluever suggested changes Dec 1, 2017

View reviewed changes

vancluever requested review from apparentlymart and removed request for apparentlymart December 1, 2017 19:18

metacpp added 4 commits December 3, 2017 20:45

Merge branch 'master' of github.com:metacpp/terraform

b639a52

Merge branch 'master' of github.com:hashicorp/terraform

3c70108

Merge branch 'master' of github.com:metacpp/terraform

f38a2a5

Add IsParallel to enable case level parallelism.

67f301d

metacpp mentioned this pull request Dec 5, 2017

Support global parallelism testing through environment variable. hashicorp/terraform-provider-azurerm#598

Closed

bflad mentioned this pull request Aug 15, 2018

helper/resource: Add ParallelTest() function to allow opt-in acceptance testing concurrency with t.Parallel() #18688

Merged

bflad added the provider-sdk label Aug 15, 2018

bflad closed this Sep 13, 2018

ghost locked and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support parallel testing in terraform testing framework #16807

Support parallel testing in terraform testing framework #16807

metacpp commented Dec 1, 2017 •

edited

Loading

vancluever commented Dec 1, 2017 •

edited

Loading

vancluever Dec 1, 2017

metacpp Dec 2, 2017

vancluever Dec 2, 2017

metacpp Dec 4, 2017 •

edited

Loading

vancluever commented Dec 2, 2017 •

edited

Loading

metacpp commented Dec 4, 2017 •

edited

Loading

metacpp commented Dec 4, 2017

apparentlymart commented Dec 4, 2017

metacpp commented Dec 5, 2017

bflad commented Sep 13, 2018

metacpp commented Sep 17, 2018

paultyng commented Sep 17, 2018

ghost commented Apr 2, 2020

Support parallel testing in terraform testing framework #16807

Support parallel testing in terraform testing framework #16807

Conversation

metacpp commented Dec 1, 2017 • edited Loading

vancluever commented Dec 1, 2017 • edited Loading

vancluever Dec 1, 2017

Choose a reason for hiding this comment

metacpp Dec 2, 2017

Choose a reason for hiding this comment

vancluever Dec 2, 2017

Choose a reason for hiding this comment

metacpp Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

vancluever commented Dec 2, 2017 • edited Loading

metacpp commented Dec 4, 2017 • edited Loading

metacpp commented Dec 4, 2017

apparentlymart commented Dec 4, 2017

metacpp commented Dec 5, 2017

bflad commented Sep 13, 2018

metacpp commented Sep 17, 2018

paultyng commented Sep 17, 2018

ghost commented Apr 2, 2020

metacpp commented Dec 1, 2017 •

edited

Loading

vancluever commented Dec 1, 2017 •

edited

Loading

metacpp Dec 4, 2017 •

edited

Loading

vancluever commented Dec 2, 2017 •

edited

Loading

metacpp commented Dec 4, 2017 •

edited

Loading