lunarmodules / busted

Elegant Lua unit testing.
https://lunarmodules.github.io/busted/
MIT License
1.41k stars 186 forks source link

_TEST only visible inside of tests #76

Closed daurnimator closed 11 years ago

daurnimator commented 11 years ago

The _TEST global is only visible inside of a test (inside an it() callback). It should be visible at the global level (i.e. when I require "mylib" at the top of the file, or inside a describe())

ajacksified commented 11 years ago

Also pinging @Tieske on this.

We can move it up all the way, but I'm worried about adding too many globals and clobbering things. What about exposing a "Busted" table at the global level that contains all the config options, _TEST, etc?

One downside is that having 'busted' code in your code code feels bad and couples your code to the testing framework, which feels bad (which is why I don't like the idea of sharing _TEST anyway.) It seems that you should either just expose your code, or you should only worry about testing the public interfaces.

Tieske commented 11 years ago

after asking on the lualist, it seems that _ALLCAPS in the global environment is against Lua conventions. So it should be changed in either busted._TEST (or equivalent other name once its inside the busted module), or something that does comply with Lua conventions like __TEST (double underscore).

I'm still in favor of a global, not a busted specific one. If some test has to test against a table somewhere then the module that exposes the locals would also have to check on the existence of the table first. eg.

if busted and busted._TEST then
  -- expose locals here
end

which I find an unnecessary complication (it also remains in production code).

It all comes down to making a choice I guess. Verdicts?

ajacksified commented 11 years ago

I'd prefer not having a global at all, because test-specific code is almost always a bad idea. I'd prefer removing it unless there is a compelling argument to having it. You should test your live code and its interfaces; if there's something that should be testable, either make it public, or test the interface. After all, that's what you're interacting with when you use your code in production, and your tests should match the usage.

Tieske commented 11 years ago

I like to test my privats :-) You could basically use the existence of the busted module table instead of the global. But then it will depend on busted, instead of a more generic global that could be used in other frameworks as well. Then again what would be the real value-add for that? so maybe just existence of the busted table will do.

ajacksified commented 11 years ago

I strongly believe that your testing environment should match the production environment as much as possible, and _TEST specific code violates that principle by allowing you to run different code that could potentially hide or introduce bugs.

As for the need to even test private code: it probably falls into one of a few categories: it's a code smell that needs refactoring, it's a pass-through to something else, or it simply shouldn't actually be private. This has some good arguments in both directions: http://blog.rubybestpractices.com/posts/gregory/034-issue-5-testing-antipatterns.html

Most prevailing wisdom in a Google search agrees that privates shouldn't be tested:

Tieske commented 11 years ago

Well, I know this is a hot debate, always was, always will be probably. In my opinion everybody should follow their own guidelines, if that includes testing private elements, then _TEST will be welcome to those. For anyone who strongly feels otherwise, there is no need to use it, if you don't want to nobody is forcing you.

So leaving it as it is doesn't do any harm, removing it makes it harder for those who want to test private elements. I say leave the choice to the user.

And other than that, it's just opinions, so if you stick to removing it, thats fine. Lets get this thread closed.

Tieske commented 11 years ago

this has been fixed in master. Can be closed.