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

Add ResetGlobal function #692

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Conversation

nogates
Copy link
Contributor

@nogates nogates commented Jun 16, 2020

Hello! First of all, thank you for this amazing project!! ❤️

We are currently using Ginkgo to dynamically generate some tests (depending on some configuration) and it works great. However, we are having some troubles to test different scenarios where different describe or context groups will be created. Since ginkgo stores everything under one global Suite struct and there is currently no way to reset that variable, it's very hard for us to test this logic (we are testing the tester, I know! :)

In this PR, I am adding a ResetGlobals function that is exposed as part of the ginkgo DSL, which just resets the globals variables to the original state, and I wanted to get some feedback before spending more time adding tests (I was going to open an Issue first, but given that little amount of code that's needed to show how this could be implemented, I decided to went ahead and open a PR instead, I hope this is ok!)

What do you think about this? Do you think we should do this in a different way?

Thank you!

@onsi
Copy link
Owner

onsi commented Jun 16, 2020

Thanks @nogates - and cool use case you have going :)

I can't think of an alternative approach. My only hesitation is including ResetGlobals in the top-level Ginkgo DSL. I think, as an alternative, this could go in the extensions folder as a new standalone package that simply exposes ResetGlobals. We should think about what to call that package - any suggestions?

@nogates
Copy link
Contributor Author

nogates commented Jun 16, 2020

Hey @onsi ! Thanks for the quick reply :)

I can't think of an alternative approach. My only hesitation is including ResetGlobals in the top-level Ginkgo DSL. I think, as an alternative, this could go in the extensions folder as a new standalone package that simply exposes ResetGlobals.

Yeah, that could work! Happy to move it there!

We should think about what to call that package - any suggestions?

well, I think globals (as in globals.Reset()), is the most descriptive one... I can't think in any other name that would be more accurate and less generic 😬

@onsi
Copy link
Owner

onsi commented Jun 16, 2020

well, I think globals (as in globals.Reset()), is the most descriptive one... I can't think in any other name that would be more accurate and less generic 😬

Fair enough - would you be up for adding documentation explaining the original use-case? Within the context of an actual test suite running globals.Reset() would be no bueno - so we want to make sure folks understand that this is intended for meta testing.

this package can be used to reset the global state of ginkgo
@nogates nogates force-pushed the nogates/add-reset-globals-func branch from 08daa2b to f2cd6a3 Compare June 17, 2020 02:45
@nogates
Copy link
Contributor Author

nogates commented Jun 17, 2020

Fair enough - would you be up for adding documentation explaining the original use-case? Within the context of an actual test suite running globals.Reset() would be no bueno - so we want to make sure folks understand that this is intended for meta testing.

Definitely! I forced pushed the branch and moved the ResetGlobals to the extensions/globals#Reset package as discussed!

Also, I added a long description to the beginning of that package explaining what is the purpose of it, and I did emphasize this functionality is not intended for normal ginkgo setups.

Let me know if all of this makes sense! Thanks a lot for the time reviewing this @onsi ! 🙇

@onsi
Copy link
Owner

onsi commented Jun 17, 2020

Great! lgtm!

@onsi onsi merged commit 3295c8f into onsi:master Jun 17, 2020
@nogates nogates deleted the nogates/add-reset-globals-func branch June 17, 2020 06:59
@nogates
Copy link
Contributor Author

nogates commented Jun 18, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants