Update tests with new exclude/skipped behavior #7245
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I updated the tests with the new behavior that we're expecting by excluding tests instead of skipping them. If any of this seems off, let me know! It makes sense to me, but it's always good to get feedback.
josevalim
reviewed
Jan 22, 2018
| @@ -158,20 +158,20 @@ defmodule ExUnitTest do | |||
| assert output =~ "4 tests, 1 failure" | |||
|
|
|||
| {result, output} = run_with_filter([exclude: [even: true]], [ParityTest]) | |||
| assert result == %{failures: 0, skipped: 1, total: 4} | |||
There was a problem hiding this comment.
Let's add :excluded to the result map.
|
One tiny comment for now: I would add the Maybe for the initial implementation, can simply replace all skip/skipped by excluded. Then you can introduce the new |
Incorporating José's feedback on the UI for this before implementing the first step. His (good) recommendation was to first move all currently "skipped" tests over to being excluded, then re-implement skipping tests with the `@tag :skip` feature in the new way, so I'm going to do that.
This introduces the concept of an "excluded" test. This test is usually excluded at the command line or in some other sort of configuration option, but is _not_ excluded by using the `:skip` tag. Right now that functionality to temporarily skip a test is disabled, but in the coming commits we're going to re-enable that.
devonestes
force-pushed
the
differentiate-between-skipped-and-excluded
branch
from
73d13fc to
9e078d7
Compare
We're now making a distinction between tests that are excluded and skipped. Tests that are skipped are only skipped by using the `@tag :skipped` tag, whereas there are many way to exclude a test. I've also re-implemented the printing of a yellow * for skipped tests in the output, as well as making the `1 skipped` message in the output yellow.
devonestes
force-pushed
the
differentiate-between-skipped-and-excluded
branch
from
9e078d7 to
e9e5715
Compare
devonestes
changed the title
|
Ok, this is all updated with the addition of a the new concept of an "excluded" test. Where do you think the best place to document this change is? |
josevalim
reviewed
Jan 22, 2018
|
|
||
| def handle_cast({:test_finished, %ExUnit.Test{state: {:skipped, _}} = test}, config) do | ||
| if config.trace do | ||
| IO.puts(trace_test_skipped(test)) |
There was a problem hiding this comment.
Yeah, totally. I'll update that.
josevalim
reviewed
Jan 22, 2018
| @@ -238,7 +256,8 @@ defmodule ExUnit.CLIFormatter do | |||
|
|
|||
| message = | |||
| "#{test_type_counts}#{config.failure_counter} #{failure_pl}" | |||
| |> if_true(config.skipped_counter > 0, &(&1 <> ", #{config.skipped_counter} skipped")) | |||
| |> if_true(config.skipped_counter > 0, &(&1 <> ", " <> skipped("#{config.skipped_counter} skipped", config))) | |||
There was a problem hiding this comment.
I think now we can color the whole thing in yellow? As you originally proposed?
There was a problem hiding this comment.
Oh, I never really wanted to color the whole thing yellow, just the 1 skipped part. Do you think the whole thing being yellow is better?
josevalim
reviewed
Jan 22, 2018
| @@ -140,7 +140,7 @@ defmodule ExUnit.Runner do | |||
|
|
|||
| case ExUnit.Filters.eval(include, exclude, tags, tests) do | |||
| :ok -> %{test | tags: tags} | |||
| {:error, msg} -> %{test | state: {:skip, msg}} | |||
| excluded_or_skipped -> %{test | state: excluded_or_skipped} | |||
|
I have added minor comments about style, everything else is perfect! |
|
❤️ 💚 💙 💛 💜 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I updated the tests with the new behavior that we're expecting by
excluding tests instead of skipping them. If any of this seems off, let
me know! It makes sense to me, but it's always good to get feedback
before I get too deep in the weeds.