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

adds filter for showing only slow tests #207

Merged
merged 8 commits into from Jan 11, 2022
Merged

Conversation

grimmjo
Copy link
Contributor

@grimmjo grimmjo commented May 13, 2021

closes #158

@grimmjo grimmjo changed the title adds filter for only showing slow tests adds filter for showing only slow tests May 13, 2021
@grimmjo
Copy link
Contributor Author

grimmjo commented Dec 8, 2021

Hey @radarsh,

any news on this?
What needs to be done to get this PR merged?

Kind regards

@radarsh
Copy link
Owner

radarsh commented Dec 10, 2021

Hi @grimmjo, only feedback I have is to rename showSlow to showOnlySlow to be very clear. Please make these changes and it can then be merged. Thank you for the PR.

Oh and while you're there, could you also document this new property in the README? Thanks.

@grimmjo
Copy link
Contributor Author

grimmjo commented Dec 10, 2021

Hi @radarsh,
renaming is done and I also added the parameter to the README. I think we should be ready to merge 😃
When do you plan to release the next version?

@radarsh
Copy link
Owner

radarsh commented Dec 16, 2021

Hi @grimmjo there seems to be a problem. I'm running the tests with showOnlySlow=true and yet it's showing all tests, slow and fast.

➜  gradle-test-logger-plugin git:(slowFilter) ./gradlew clean test -Dtestlogger.showOnlySlow=true -Dtestlogger.slowThreshold=100

> Task :test

  com.adarshr.gradle.testlogger.renderer.AnsiTextRendererSpec

    render #input

      ✔ render [red]text (130ms)
      ✔ render [bright-red]text
      ✔ render [foo]text
      ✔ render [red,foo]text
      ✔ render [yellow]text
      ✔ render [bright-yellow]text
      ✔ render [green]text
      ✔ render [bright-green]text
      ✔ render [bold][red]text[/]
      ✔ render [bold,red]text[/]
      ✔ render [erase-all,bold,red]text[/]
      ✔ render [erase-ahead,bold,red]text[/]
      ✔ render [erase-back,bold,red]text[/]
      ✔ render [cursor-up-line]text[/]
      ✔ render [red][red][/]
      ✔ render [bold]text[bold-off]
      ✔ render [default]text
      ✔ render [grey]text
      ✔ render [cyan]text
      ✔ render [bright-cyan]text
      ✔ render [blue]text
      ✔ render [bright-blue]text
      ✔ render [magenta]text
      ✔ render [bright-magenta]text

  com.adarshr.gradle.testlogger.logger.OutputCollectorSpec

    ✔ output collector collects output by descriptor ID and removes them accordingly (912ms)

  com.adarshr.gradle.testlogger.logger.TestLoggerWrapperSpec

    ✔ wrapper delegates to sequential test logger if parallel theme is not applied (55ms)
    ✔ wrapper delegates to parallel test logger if parallel theme is applied

  com.adarshr.gradle.testlogger.logger.ConsoleLoggerSpec

    ✔ log
    ✔ does not log empty strings
    ✔ log new line
    ✔ configurable log level

README.md Outdated
}
```

By default all the above three flags are turned on. If you have chosen to display standard streams by setting
By default all the above four flags are turned on. If you have chosen to display standard streams by setting
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showOnlySlow is will be off by default. So this goes back to being

By default the flags showPassed, showSkipped and showFailed are turned on while showOnlySlow will be off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ;) just inserted your quote.

@@ -21,7 +21,8 @@ class TestResultWrapper {
boolean isLoggable() {
testLoggerExtension.showPassed && testResult.successfulTestCount ||
testLoggerExtension.showSkipped && (testResult.resultType == SKIPPED || testResult.skippedTestCount) ||
testLoggerExtension.showFailed && testResult.failedTestCount
testLoggerExtension.showFailed && testResult.failedTestCount ||
testLoggerExtension.showOnlySlow && (isTooSlow() || isMediumSlow())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something wrong with this expression.

Copy link
Owner

@radarsh radarsh Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to get it to work by changing the method body to be as below. If you can do it in a more readable manner, please do so.

if (testLoggerExtension.showOnlySlow && (!isTooSlow() || !isMediumSlow())) {
    return false
}
 
testLoggerExtension.showPassed && testResult.successfulTestCount ||
    testLoggerExtension.showSkipped && (testResult.resultType == SKIPPED || testResult.skippedTestCount) ||
    testLoggerExtension.showFailed && testResult.failedTestCount

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored this method a little bit to make it more readable

@radarsh
Copy link
Owner

radarsh commented Dec 16, 2021

My understanding of this flag is:

Given showOnlySlow=true and slowThreshold=5000
And I have a test "test-1" that takes "10000" ms to run
And I have a test "test-2" that takes "3000" ms to run
When I run the tests
Then I should see "test-1" printed

and

Given showOnlySlow=false and slowThreshold=5000
And I have a test "test-1" that takes "10000" ms to run
And I have a test "test-2" that takes "3000" ms to run
When I run the tests
Then I should see "test-1" and "test-2" printed

@@ -227,6 +229,15 @@ class TestLoggerExtension extends TestLoggerExtensionProperties {
.getOrElse(false)
}

Boolean getshowOnlySlow() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the method name :) This is why the command line argument wasn't working. It should be getShowOnlySlow (capitalised).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Done ;)

@@ -127,6 +127,29 @@ class TestResultWrapperSpec extends Specification {
10001 | false
}

@Unroll
def "loggable returns false if showOnlySlow is turned #showOnlySlow"() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests will need fixing too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@grimmjo
Copy link
Contributor Author

grimmjo commented Dec 17, 2021

My understanding of the flag is a little bit different as you may see in the isLoggable method.
First we need to check if either one of the flags showPassed, showSkipped or showFailed should be applied.
The showOnlySlow then filters the results from above.

What we should think about is, that we only should filter the passed test as the consumer may wants to see always the skipped and failed tests.

@grimmjo grimmjo requested a review from radarsh December 17, 2021 08:13
@radarsh
Copy link
Owner

radarsh commented Dec 17, 2021

You make a very good point here.

What we should think about is, that we only should filter the passed test as the consumer may wants to see always the skipped and failed tests.

We shouldn't hide failing tests because they failed fast :) But your PR seems to be already doing this.

@grimmjo
Copy link
Contributor Author

grimmjo commented Jan 3, 2022

Hi @radarsh 😃

belated happy new year ;) Do you have any plans on merging this pr and releasing a new version?

@radarsh
Copy link
Owner

radarsh commented Jan 5, 2022

@grimmjo Happy New Year. Yeah, let me get this done this week.

@radarsh radarsh merged commit af38e98 into radarsh:develop Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filter: hide regular passing tests, show slow passing tests
2 participants