Support ExUnit filters #1909
Support ExUnit filters #1909
Conversation
There was a problem hiding this comment.
I don't think we need to store them, since we use it only on boot. :)
There was a problem hiding this comment.
Oh good call. We can just print them straight from init/1.
Yeah, let's not make this assumption. Let's check explicitly with
The way you tackled this was actually pretty good. That's pretty much how we have to do it now.
I think this is a good idea. Go for it!
No, this is actually how it was supposed to work. :) I have added some comments as well, this is looking really good! Thanks! |
|
Would this fit my use case for Erjang? I'd like to be able to disable/enable specific tests for erjang. |
|
@krestenkrab exactly! |
Cool. Sounds good. Do we need a way for |
Not from now. But it can always be set directly via |
lib/mix/lib/mix/tasks/test.ex
Outdated
There was a problem hiding this comment.
Can we add a section to this documentation named ## Filters explaining what is the supported command line syntax and linking to ExUnit.Case docs for more information?
There was a problem hiding this comment.
Similarly, we should update the tags section in the ExUnit.Case module docs to mention filters and simply say options can be passed via command line as well with mix test. :)
|
List of things to fix:
I hope I didn't muck this up too much, but I also changed how filters work slightly compared to #1898 to match how they are implemented in rspec. Specifically this part changed slightly:
Now when defining an inclusion filter (the normal version without the Let me know how you feel about the change or if a better example would make it more clear. I'd be happy to switch it back over to what was originally spec'd out. Thanks for all the feedback so far! |
We should ignore the tags that the filter does not concern, i.e. if you filter We could support your proposal through |
|
@ericmj Cool I'll go ahead and switch that back. When using an exclusion filter, |
Yes 👍 |
There was a problem hiding this comment.
Ok, now we are actually printing those filters as tuples. Maybe we should consider a different way to print that?
|
Sweet @vanstee! The issue with supporting Any suggestions? Because I don't have any. :D |
|
@thomas-holmes and I were discussing how filters should work with multiple tags. Here are some examples we walked through: With the current rules I would assume all tests that either don't have an When using multiple filters with different tags, I think most people would expect these rules to be anded together. So for this test, I would expect the following results:
Is this what you had in mind? Also it might be nice for either the |
|
That's a very good point and I agree with the semantics! The only remaining concern right now is the configuration options (i.e. how we are going to configure but not through the command line) and how to print the filters with different combinations. Here is one approach: We can provide a ExUnit.configure reject: [type: "slow"], filter: [os: "unix"]Running tests would print: Then, through the command line, we can get rid of the Another approach would be to replace the words filter and reject by including and excluding. The end result is the same, just how we are going to call them changes. How does this sound? |
|
I like it. I'll throw this together and see how it looks. |
|
I'd prefer using include/exclude rather than filter/reject, as the meaning is much more clear. In particular the word "filter" could be interpreted to mean either include or exclude. Sent from my iPhone On 07/12/2013, at 11.03, "José Valim" <notifications@github.commailto:notifications@github.com> wrote: That's a very good point and I agree with the semantics! The only remaining concern right now is the configuration options (i.e. how we are going to configure but not through the command line) and how to print the filters with different combinations. Here is one approach: We can provide a reject option besides the filter one: ExUnit.configure reject: [type: "slow"], filter: [os: "unix"] Running tests would print: Filtering tags: [os: "unix"] Then, through the command line, we can get rid of the ~ semantics in favor of explicit --filter and --reject options. If an option is given more than once, it is accumulated and has or semantics. Another approach would be to replace the words filter and reject by including and excluding. The end result is the same, just how we are going to call them changes. How does this sound? — |
|
@vanstee any news? :) |
|
@josevalim Still need to finish up the documentation and message when running tests. How does this look so far? |
lib/ex_unit/lib/ex_unit/runner.ex
Outdated
There was a problem hiding this comment.
@josevalim Any thoughts on how to improve this? The permutation part at the beginning and the reduce might be a little hard to follow.
There was a problem hiding this comment.
FWIW, the reduce seems perfectly clear to me, absolutely out of context. It’s a common pattern for dictionary iteration/building. As I understand it you’re trying to do two things in the reduce:
- consolidate duplicate keys, favoring order of insertion but preferring truthy values
- convert a ListDict to a HashDict
HashDict.new accepts ListDicts and a transformation function and the Keyword module has utilities for handling ListDicts with duplicate keys, but scanning through their docs I don’t think there’s anyway you could use them to get the same behaviour while only iterating over the collection once.
As for the permutation stuff, I’d use list comprehensions:
results = lc filter inlist Dict.keys(filters), tag inlist Dict.keys(tags), do: evaluate_filter(filter, tag)
Christopher Keele
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
On Monday, December 16, 2013 at 1:54 PM, Patrick Van Stee wrote:
In lib/ex_unit/lib/ex_unit/runner.ex:
- Enum.map filters, fn filter -> > + { { elem(filter, 0), elem(tag, 0) }, evaluate_filter(filter, tag) } > + end > + end > + > + results = Enum.reduce results, HashDict.new, fn { key, evaluation }, dict -> > + Dict.update dict, key, evaluation, &(evaluation || &1) > + end > + > + mismatch = Enum.find Dict.to_list(results), &match?({ _, false }, &1) > + > + case mismatch do > + { { _, tag }, _ } -> { :error, tag } > + _ -> :ok > + end > + end
Any thoughts on how to improve this? The permutation part at the beginning and the reduce might be a little hard to follow.—
Reply to this email directly or view it on GitHub (https://github.com/elixir-lang/elixir/pull/1909/files#r8379268).
There was a problem hiding this comment.
FWIW, the reduce seems perfectly clear to me
Sweet!
...but scanning through their docs I don’t think there’s anyway you could use them to get the same behaviour while only iterating over the collection once.
Yeah I was looking for some kind of merge function to use as well but this seemed like a decent alternative.
As for the permutation stuff, I’d use list comprehensions:
Good call! Thanks.
|
Almost done. Just need to improve the docs a little more. Let me know if you have suggestions to improve anything implementation related. |
|
@vanstee this looks awesome! Let us know when it is ready and so we can merge it! |
|
@josevalim I think this is finally done! Feel free to make any changes. Hope the docs aren't too verbose. 💚 |
|
Thank you! I will merge it soon and it will be in the next release! :) |
|
Merged, thank you! ❤️ |
5 participants
nil. Let me know if we need to allow it.~prefix similar to rspec? Might be nice to just use the tag name combined with the exclusion fortrueandfalse(win32or~unix). Then everything else could be a string by default instead of needing a--filter os:unix:style filter.ExUnit.configureafter thetest_helper.exsis run. Any scenarios where that would be a problem?Closes #1898