Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add tests= filter for eunit suites #282

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

ostinelli commented Jul 29, 2012

Following implementation discussion in issue #250.

To summarize, consider the following setup:

myapp_mymod.erl

-module(myapp_mymod).
-export([myfunc/0]).
-include_lib("eunit/include/eunit.hrl").
myfunc() -> ok.
myprivate_test() -> ?assert(true).

myapp_mymod_tests.erl

-module(myapp_mymod_tests).
-compile([export_all]).
-include_lib("eunit/include/eunit.hrl").
myfunc_test() -> ?assertMatch(ok, myapp_mymod:myfunc()).

rebar eunit suites=myapp_mymod will run:

  • myapp_mymod:myprivate_test/0
  • myapp_mymod_tests:myfunc_test/0

rebar eunit suites=myapp_mymod tests=my will run the first match:

  • myapp_mymod:myprivate_test/0

rebar eunit suites=myapp_mymod tests=myfun will run the first match (available in _tests module):

  • myapp_mymod_tests:myfunc_test/0
Contributor

ostinelli commented Jul 29, 2012

@tuncer, @dizzyd and @richcarl, this should be what you guys had in mind.

@ghost ghost assigned tuncer Jul 29, 2012

It's not apparent from the example if this works for generator functions as well (or a mix of simple tests and generators), but I assume that it does. If the example is going into the docs you might want to fix that.

Contributor

ostinelli commented Jul 30, 2012

@richcarl indeed, eunit:test([eunit_test:function_wrapper(M, F)]) works for both. Since these are all tests, why do you believe we should differentiate in the example?

Also, I've just realized that:

rebar suites=foo_tests tests=bar

will not work because _tests modules are currently stripped out at the beginning of the parsing rebar does to avoid running these tests twice. However if you do:

rebar suites=foo tests=bar

then foo_tests:bar/0 will be run, but only if there isn't a foo:bar/0 which will otherwise be run instead.

The question is: do you think we should cover this case too, i.e. allowing the developer to specify that they can select a specific test in the _tests module too? If so, I can provide a patch and rebase it to this one. Not sure what happens to the cover cases though.

Contributor

ferd commented Aug 2, 2012

I have to say that I frequently write functions that test other implementations with similar names in the main module, but they nearly never intersect, mostly due of needing the _test() or _test_() name.

If this pull request changes the behaviour so that functions that do not end in either of these two patterns can be run as tests, I'm not sure I would want to encourage it. It would be changing a long-established and stable eunit behaviour for something entirely new and non-obvious. Suddenly, tests that are defined in the module and runnable in a way are no longer runnable in another one as they are ignored.

Related to this, this is Eunit's standard structure in test descriptors for eunit:test/1-2 :

  • {module, Mod} runs all tests in Mod
  • {dir, Path} runs all the tests for the modules found in Path
  • {file, Path} runs all the tests found in a single compiled module
  • {generator, Fun} runs a single generator function as a test.
  • {application, AppName} runs all the tests for all the modules mentioned in AppName's .app file.

While the {generator, Fun} pattern would allow what this pull request does, I think (and @richcarl can confirm/deny) it was meant to run individual tests rather than change eunit's behaviour, more as a way to debug existing suite than to allow someone to work around how Eunit expects tests to be named.

It could make sense to restrict its use a bit, or at the very least to use documentation the follows the _test() or _test_() standards rather than foo.

Contributor

ostinelli commented Aug 2, 2012

If this pull request changes the behaviour so that functions that do not end in either of these two patterns can be run as tests, I'm not sure I would want to encourage it.

It does not. It only considers exported functions ending in _test or _test_.

It could make sense to restrict its use a bit, or at the very least to use documentation the follows the _test() or _test_() standards rather than foo.

In the original pull request #250 you can see that the 'shortest matching logic' has been requested. This is documented here and here.

tl;dr: tests=foo will only match test functions starting with foo (i.e. foo*_test and foo*_test_).

Related to this, this is Eunit's standard structure in test descriptors for eunit:test/1-2 :

Following the above mentioned discussion, FYI @richcarl has already added {test, M, F} to the descriptors, in his repo.

Moreover, the usage of eunit_test:function_wrapper(M, F) in the patch follows his recommendation.

Contributor

ostinelli commented Aug 4, 2012

Finalized and updated patch.

To summarize:


rebar eunit suites=foo

will run all tests in foo and foo_tests.


rebar eunit suites=foo_tests

will run all tests in foo_tests.


rebar eunit suites=foo tests=bar

will look in this order, and run only the first existing test:

foo:bar_test
foo:bar_test_
foo_tests:bar_test
foo_tests:bar_test_

rebar eunit suites=foo tests=b

same as previous example, will look first into foo and then into foo_tests to run only the first existing test whose name starts with b.


rebar eunit suites=foo_tests tests=bar

will look in this order, and run only the first existing test:

foo_tests:bar_test
foo_tests:bar_test_

rebar eunit suites=foo_tests tests=bar_test_

will look in this order, and run only the first existing test:

foo_tests:bar_test_

richcarl commented Aug 6, 2012

Seems good to me. The only thing you might want to consider is whether the user should be allowed to give a regexp or glob instead of just a prefix, making it possible to select a bunch of tests easily, e.g., tests=foo* or tests=(foo|bar)_blaha.*

Contributor

ferd commented Aug 7, 2012

@ostinelli that seems good to me.

@richcarl That's an interesting idea. Would it create any command line issues if I have a function with a 'name-as*-atom-with-*-in-it_test'? It might be better to document the fuzzy matching as expected behaviour (or risk people not knowing about it) rather than starting to handle escaping in there, though that's nothing but a corner case.

Contributor

ostinelli commented Aug 7, 2012

@richcarl I believe this is a good idea. However, this pull request has been going on for more than 2 months now (see #250), and I'd like to see it merged upstream, or abandoned.

What you are proposing can easily be implemented in a future feature, since it wouldn't break backwards compatibility with the functionality currently implemented in this patch.

I guess now it's up to @tuncer and the basho guys. ^^_

r.

Contributor

tuncer commented Aug 11, 2012

Thanks, merged.

@tuncer tuncer closed this Aug 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment