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

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

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Aug 15, 2018

Please note: this is an alternate implementation to #16807, using a separate function rather than requiring adjustment to each existing TestCase and submitted just for an additional reference point. The preferred solution may be either implementation or a combination of the two! I think more importantly we should continue the conversation for supporting this type of functionality as its been implemented or requested by a few of the major providers. 😄

While this initial implementation is a very simple wrapper function, implementing this in the helper/resource package provides some downstream benefits:

  • Provides a standard interface for plugin developers to enable parallel acceptance testing
  • Existing plugins can simply convert resource.Test to resource.ParallelTest references (as appropriate) to enable the functionality, rather than worrying about additional line(s) to each acceptance test function or TestCase
  • Potential enhancements to ParallelTest (e.g. adding an environment variable to skip enabling the behavior) are consistently propagated

…ce testing concurrency with t.Parallel()

While this initial implementation is a very simple wrapper function, implementing this in the helper/resource package provides some downstream benefits:
* Provides a standard interface for plugin developers to enable parallel acceptance testing
* Existing plugins can simply convert resource.Test to resource.ParallelTest references (as appropriate) to enable the functionality, rather than worrying about additional line(s) to each acceptance test function or TestCase
* Potential enhancements to ParallelTest (e.g. adding an environment variable to skip enabling the behavior) are consistently propagated
@paddycarver
Copy link
Contributor

I can't believe I didn't think of that before adding t.Parallel() to hundreds of tests. 😓

@MalloZup
Copy link

MalloZup commented Sep 10, 2018

@bflad what is the next step for this PR to get merged? i really like this feature and could help us in our terraform libvirt provider having this to the schema helper

Thank you for your attention. Have nice day.! 🌞 🌻

@bflad bflad requested a review from a team September 10, 2018 16:31
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, providers can opt-in 1 test at a time. I'm not an authority on core though so I would prefer see another approval maybe? But I will get it started.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This doesn't have any implications for Terraform Core, but indeed LGTM!

@MalloZup
Copy link

Thanks :love:

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bflad
Copy link
Contributor Author

bflad commented Sep 13, 2018

Thanks, everyone!! 🚀

@bflad bflad merged commit a47583d into master Sep 13, 2018
@bflad bflad deleted the f-resource-paralleltest branch September 13, 2018 15:25
bflad added a commit that referenced this pull request Sep 13, 2018
@MalloZup
Copy link

🚀 🎸 🎉

@metacpp
Copy link

metacpp commented Sep 17, 2018

@bflad thanks for the fantastic work!

One more thing to take care: the output of testing result is a known issue for go test running in parallel. Please visit golang/go#19280 for the details. The proposal is to output test result in a structural (for example, JSON) format, see golang/go#2981. And it was released in v1.10 through go test -json, we need to remind provider developer for this.

@ghost
Copy link

ghost commented Apr 2, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants