Conversation
# Conflicts: # docs/RELEASE_NOTES.md
ahmetoz
left a comment
ahmetoz
left a comment
There was a problem hiding this comment.
Good work, it seems you fixed the important issues in BenchmarkUtils. 🚀 🎉
| + " threshold of '%d'.", diff, THRESHOLD)) | ||
| .isLessThanOrEqualTo(THRESHOLD); | ||
| // assert on threshold (based on history of benchmarks; highest was ~9 seconds) | ||
| final int threshold = 69000; // 1 minute higher than highest benchmark |
There was a problem hiding this comment.
Comment "1 minute" is confusing as it is not exact in comparison to the actual value. Also why would you use such high threshold if from history you know that it was never beyond 9s? Maybe 18s would be better threshold value here? Same comment applies to all other tests below.
There was a problem hiding this comment.
- Maybe instead of 1 minute -> ~1 minute in the comment?
Also why would you use such high threshold if from history you know that it was never beyond 9s?
makes sense. Will change.
There was a problem hiding this comment.
Maybe instead of 1 minute -> ~1 minute in the comment
Why not the truth right away? :) -> "69 seconds"
There was a problem hiding this comment.
Just to explain the choice of the magic number :)
| as (`true`, `SingleLine` and `None` respectivley) if they are `null` or not passed. [#354](https://github.com/commercetools/commercetools-sync-java/issues/354) | ||
|
|
||
| - 🛠️ **Enhancements** (1) | ||
| - **Commons** - Benchmarks are now run once on every merge to `master` with a lower number of resources for faster benchmarking. [#246](https://github.com/commercetools/commercetools-sync-java/issues/246) |
There was a problem hiding this comment.
Why not run it on all PR branches too as one would like to see regression of performance before actual merge to master, right? How long does it take now?
There was a problem hiding this comment.
I get that it will be useful for a maintainer, but not for a lib user.
hmm, the problem with that Is the graph would be piled up quickly with new results for every single PR opened.
Also shouldn't the graph show the benchmarks of the actual state of the library (on master) for the users using it as opposed to just a PR that a dev is working on?
How long does it take now?
Still didn't try a full benchmark run on travis with this change. We'll see after this is merged.
There was a problem hiding this comment.
I get that it will be useful for a maintainer, but not for a lib user
Any contributor would probably use the library too - stable performance is relevant to everyone.
hmm, the problem with that Is the graph would be piled up quickly with new results for every single PR opened
Also shouldn't the graph show the benchmarks of the actual state of the library (on master) for the users using it as opposed to just a PR that a dev is working on?
Maybe run the benchmark on any PR, report readable regression but update benchmark JSON only on master. wdyt?
There was a problem hiding this comment.
This would be good, yes. Will add to the improvements issue #246 and address later then ;)
There was a problem hiding this comment.
Codecov Report
@@ Coverage Diff @@
## master #364 +/- ##
============================================
- Coverage 99.26% 99.25% -0.01%
+ Complexity 1261 1259 -2
============================================
Files 112 112
Lines 3116 3106 -10
Branches 152 152
============================================
- Hits 3093 3083 -10
Misses 12 12
Partials 11 11
Continue to review full report at Codecov.
|
4 participants
Summary
Addresses some points in #246
Description
Merge pull request... -> which means on every merge to master basically.Relevant Issues
#246
Todo
~- [ ] Unit ~
- [ ] Integration- [ ] Documentation