-
Notifications
You must be signed in to change notification settings - Fork 98
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
make test parameters visible to setup method #119
Comments
Not sure I understand why you should have this. This is pretty normal in unit test frameworks as well. |
@ewencp The point I'm making is that users now have started effectively awkwardly splitting setup logic into two methods, and this is unnecessary. Here is what I see happening now:
vs
I'd argue that this is quite a bit more clear, readable, and simple. Although there is some implicit magic in how the parameters get injected into setup, to me this seems pretty intuitive. |
@granders They don't, but it doesn't really change anything. I just meant that if you were doing anything that requires a standard pattern in your test, but customization per test, then there will be some duplication, e.g. to mirror your example:
If it's worth it, you would just refactor those lines into a method you could call. Part of the value of setUp is that it should also help you tell the difference between the thing you're testing failing and something unrelated breaking that is causing your test to fail. But other than that, it's really just a formalized way to express common code and avoid 1 method call. In any case, think about what this change would mean when you don't have the exact same set of parameters. For example, take a look at benchmark_test in Kafka. |
A funky pattern has started to develop in kafka system tests, which means that
setup
isn't quite doing its job:start_kafka
etc which are effectively shared setup logic that make use of these parameters, but which have to be called directly in every test methodThe behavior we might want:
The text was updated successfully, but these errors were encountered: