Skip to content

Add ResetGlobal function#692

Merged
onsi merged 1 commit into
onsi:masterfrom
nogates:nogates/add-reset-globals-func
Jun 17, 2020
Merged

Add ResetGlobal function#692
onsi merged 1 commit into
onsi:masterfrom
nogates:nogates/add-reset-globals-func

Conversation

@nogates

@nogates nogates commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

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

onsi commented Jun 16, 2020

Copy link
Copy Markdown
Owner

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

nogates commented Jun 16, 2020

Copy link
Copy Markdown
Contributor Author

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

onsi commented Jun 16, 2020

Copy link
Copy Markdown
Owner

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

nogates commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

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

onsi commented Jun 17, 2020

Copy link
Copy Markdown
Owner

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

nogates commented Jun 18, 2020

Copy link
Copy Markdown
Contributor Author

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.

2 participants