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

Allow settting up custom threshold for different tasks #94

Merged
merged 2 commits into from
Jun 5, 2021

Conversation

klebekj
Copy link
Contributor

@klebekj klebekj commented May 23, 2021

Within my business project, we use pretty-jupiter and display test logs while building. We have both unit and integration tests and they have significant duration difference. Setting up duration.threshold is not enough for this case. I've implemented customThreshold map in extension to store task name (e.g 'test' or 'integrationTest') and custom threshold value assigned to it. Custom threshold, when currently running task is present in map, overrides duration.threshold and gives the opportunity for better test logs customization.

@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #94 (a70ae8f) into master (dba0391) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              master       #94   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity        49        49           
===========================================
  Files              7         7           
  Lines            147       157   +10     
  Branches          12        14    +2     
===========================================
+ Hits             147       157   +10     
Impacted Files Coverage Δ
...selion/prettyjupiter/PrettyJupiterExtension.groovy 100.00% <100.00%> (ø)
.../github/joselion/prettyjupiter/PrettyLogger.groovy 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dba0391...a70ae8f. Read the comment docs.

Copy link
Owner

@JoseLion JoseLion left a comment

Choose a reason for hiding this comment

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

@klebekj first of all, thanks a lot for your contribution. I'm so glad to hear people are finding this plugin useful, and it's so great we can keep increasing its features!

I think the customThreshold is a great idea, I just wonder if it'd be possible to use a closure instead of a map, something like this:

prettyJupiter {
  duration {
    enabled = true
    customThreshold {
      test = 100
      integrationTest = 150
    }  
  }
}

I feel like that'd be a more straightforward way of configuring the thresholds, but TBH I haven't thought about how that could be achieved. What do you think? 🙂

Other than that, I think everything looks good. I only left a few comments regarding code style, nothing major though. Thanks for keeping the test coverage 💯

@klebekj
Copy link
Contributor Author

klebekj commented Jun 2, 2021

Thanks for your review, I resolved all your comments.
About using closure instead of a map - I agree with you that it looks better and I tried to implement this but unfortunately I'm not able to achieve it :(

@JoseLion
Copy link
Owner

JoseLion commented Jun 5, 2021

Awesome! Yeah, I wasn't sure how easy/possible would that be 😅. However, I think the map is a good solution too! It's even defined as a lazy property which is a good point in favor 🙂

Copy link
Owner

@JoseLion JoseLion left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks a lot for your contribution 🎉

@JoseLion JoseLion merged commit d32b4a7 into JoseLion:master Jun 5, 2021
@klebekj klebekj deleted the feature/custom-threshold-per-task branch June 7, 2021 10:22
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.

2 participants