Skip to content
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

Add a toggle command+setting to hide Skipped tests in the test tree #3038

Closed
timsneath opened this issue Dec 30, 2020 · 11 comments · Fixed by #3084
Closed

Add a toggle command+setting to hide Skipped tests in the test tree #3038

timsneath opened this issue Dec 30, 2020 · 11 comments · Fixed by #3084
Labels
in testing Relates to test execution of Dart/Flutter tests for end users is enhancement
Milestone

Comments

@timsneath
Copy link
Contributor

Describe the bug
In my test suite, I have lots of tests that are currently skipped (they are optional tests in an emulator suite for undocumented behavior). While they appear with a yellow pause icon, indicating that they've been skipped (I think), they show as "not passed", and there's no easy way I could find to filter them out from the overall test pass count .
image

To Reproduce
Clone and open this commit: https://github.com/timsneath/cambridge/tree/bd92928b6566fb0531e5c4045671b8ac277ecadd
And then run Dart : Run All Tests

Screenshots
image

(Running latest version of DartCode / Flutter master channel...)

@DanTup
Copy link
Member

DanTup commented Jan 1, 2021

Do you think it would be better to include each category in the label (100 pass, 15 skip, 6 fail)? That seemed quite long, but it's probably a little confusing to include the skipped results in either the passes or fails (and excluding them entirely from the total count may also seem strange).

I should probably find better icons too.. I was never really happy with these and hoped VS Code would get some standard icons (since many extensions implement their own test trees), but it didn't. Not sure what a good icon/color for Skipped would be though, so I'm open to suggestions if you have any!

@DanTup DanTup added this to the v3.19.0 milestone Jan 1, 2021
@DanTup DanTup added in testing Relates to test execution of Dart/Flutter tests for end users is enhancement and removed is bug labels Jan 1, 2021
@timsneath
Copy link
Contributor Author

(Personal opinion, not an official Dart team response!)

I come back to the principle of skipped tests as described in the test package:

If a test, group, or entire suite isn't working yet and you just want it to stop complaining, you can mark it as "skipped".

All these skipped tests make it very hard for me to quickly identify the failed tests, which is what really matters.

So I'd prefer to see them omitted altogether from the list and instead just include a mention in the header line, e.g. something like:

750/786 tests passed (732 tests skipped)

Others might have a different perspective though.

@timsneath
Copy link
Contributor Author

I assume that there's no opportunity right now to have a toggle button in the header area that lets you choose whether skips are displayed?

@DanTup
Copy link
Member

DanTup commented Jan 1, 2021

750/786 tests passed (732 tests skipped)

Sounds reasonable to me, though it may be better to drop the word "tests" to avoid pushing the duration off too easily.

I assume that there's no opportunity right now to have a toggle button in the header area that lets you choose whether skips are displayed?

I think buttons can be added along the top (similar to where the Collapse button would be in the Explorer) although if changed to the above, I'm not sure if there would be much value in that?

@timsneath
Copy link
Contributor Author

Your suggestion sounds great. But I was thinking that it's useful to be able to easily filter these kinds of things out. I think I remember VS (proper, not VSCode) having a toolbar that let you quickly clear all tests, run skipped tests, sort by failed tests, etc. (Of course, arguably a lot of this should come from VSCode rather than the extension, but I'm not sure where the dividing line is.)

@DanTup
Copy link
Member

DanTup commented Jan 5, 2021

Oh - I may have misunderstood. I was looking at just the label, though I think you want to hide the nodes too 😄

So, I think this makes

  • Change the node text to x/y passed (+z skipped) where x=passed count, y=passed+failed count, z=skipped count
  • Add a toggle button for "Show skipped tests" (shown by default) which
    • Drops the (+z skipped) text
    • Hides all skipped nodes from the tree (including groups/parents where all children were skipped)

I think I remember VS (proper, not VSCode) having a toolbar that let you quickly clear all tests, run skipped tests, sort by failed tests, etc

A button to clear has come up in the past, though mostly as a workaround for clearing "old" tests after renames. This happens automatically if you re-run the entire file, though if we're adding some buttons to the toolbar and this wouldn't be the only one, it probably makes sense to add now too.

We do also already have a command in the command palette for "run failed tests", so adding "run skipped tests" might not be much effort, and if we're adding toolbar buttons we could add those both there too.

I've split this into multiple issues to make it easier to track (and call out in release notes). This one will be for adding a command to toggle visibility of Skipped tests (including a setting to persist it to), and I've also opened:

arguably a lot of this should come from VSCode rather than the extension, but I'm not sure where the dividing line is

Right now, beyond the test icon for the main sidebar, none of this comes from VS Code and everything is custom (there's an open issue for something, but no progress).

@DanTup DanTup changed the title Skipped tests can't be filtered out Add a toggle command+setting to hide Skipped tests in the test tree Jan 5, 2021
@timsneath
Copy link
Contributor Author

Splendid! Yeah, I'm not being a very good product manager here :) since this was really me tinkering with code over the winter break. But you've done a great job of distilling my loosely expressed feedback into a great set of actions -- I think the four issues here would hugely enhance this feature.

(On the plus side, I've now got all those tests working on my project ;)

@DanTup
Copy link
Member

DanTup commented Jan 5, 2021

Great, sgtm!

(On the plus side, I've now got all those tests working on my project ;)

😄 Maybe you'll still find the changes useful for next years winter break project then! 🙂

DanTup added a commit that referenced this issue Jan 20, 2021
Needs wiring up to affect the display. Progress towards #3038.
DanTup added a commit that referenced this issue Jan 23, 2021
Needs wiring up to affect the display. Progress towards #3038.
DanTup added a commit that referenced this issue Jan 23, 2021
DanTup added a commit that referenced this issue Jan 23, 2021
Needs wiring up to affect the display. Progress towards #3038.
DanTup added a commit that referenced this issue Jan 23, 2021
@DanTup
Copy link
Member

DanTup commented Jan 23, 2021

This still needs a bit of work - the test counts aren't taking into account whether the skipped tests are being shown or not.

@DanTup DanTup reopened this Jan 23, 2021
@DanTup DanTup closed this as completed in b162e87 Jan 25, 2021
@DanTup
Copy link
Member

DanTup commented Jan 25, 2021

@timsneath there's a beta release if you want to try it here:

https://github.com/Dart-Code/Dart-Code/releases/tag/v3.19.0-beta.1

It includes new icons on the test runner toolbar to run all/run failed/run skipped tests, and an overflow menu with an option to toggle visibility of skipped tests (note: if an entire suite is skipped, the suite node will still display so you can access it's context menu to get the "run skipped" option for just that suite).

When you run skipped tests (either using a "run skipped" command, or by running the skipped test explicitly from CodeLens or the test node), we'll send --run-skipped so the test actually runs (rather than just being skipped again).

If you have any feedback, do send it over! :-)

@DanTup
Copy link
Member

DanTup commented Jan 29, 2021

Note: The ability to run skipped tests is being reverted due to #3107 (a patch will go out shortly). flutter test does not support --run-skipped so this needs be conditional based on the type of project each test node/CodeLens item is in (which can be done, but will take longer than it's reasonable to leave this bug).

The other stuff (such as showing/hiding the skipped tests) will remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in testing Relates to test execution of Dart/Flutter tests for end users is enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants