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 overhead time to junit test case report #684

Merged
merged 3 commits into from
Mar 30, 2020

Conversation

jan-goral
Copy link
Contributor

@jan-goral jan-goral commented Mar 27, 2020

Fixes #557

Checklist

  • Unit tested
  • release_notes.md updated

@jan-goral jan-goral self-assigned this Mar 27, 2020
@bootstraponline
Copy link
Contributor

awesome!

@jan-goral
Copy link
Contributor Author

Hey @bootstraponline there is one think to consider here.
Our new junit report is uploaded to bucket only when option smart-flank-gcs-path is used. That means to use smart sharding correctly we need to run flank twice with smart-flank-gcs-path. First run will generate and upload correct report so second run can use it to properly calculate shard times. I it correct approach? Or maybe new junit report should be uploaded always, no matter if smart-flank-gcs-path used or not.

@jan-goral
Copy link
Contributor Author

Second thing is that test suites names in junit report may not be unique so method mergeTestTimes here may not work correctly. For now I have not idea how to specify unique names to identify same suites between two test runs. But I guess that we can adjust implementation to omit suite names matching and base only on testcase names (and class names) from all suites. But them also may not be unique if user will add additional test apks where both contains same test name. Do you have any suggestions?

@codecov-io
Copy link

Codecov Report

Merging #684 into master will increase coverage by 0.10%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #684      +/-   ##
============================================
+ Coverage     77.30%   77.41%   +0.10%     
- Complexity      624      626       +2     
============================================
  Files           109      109              
  Lines          2529     2541      +12     
  Branches        360      361       +1     
============================================
+ Hits           1955     1967      +12     
  Misses          345      345              
  Partials        229      229              

@@ -176,7 +176,9 @@ object ReportManager {

val oldTestResult = GcStorage.downloadJunitXml(args)

newTestResult.mergeTestTimes(oldTestResult)
if (args.useLegacyJUnitResult) {
newTestResult.mergeTestTimes(oldTestResult)
Copy link
Contributor Author

@jan-goral jan-goral Mar 30, 2020

Choose a reason for hiding this comment

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

I've decided to omit this call when not useLegacyJUnitResult cause new junit result (from api) does not favor success over failure nor error, so we don't need to override failed test cases times.

@jan-goral jan-goral marked this pull request as ready for review March 30, 2020 03:50
pawelpasterz
pawelpasterz previously approved these changes Mar 30, 2020

private fun List<TestCase>.sumTime(): Double = this
.map { it.elapsedTime.millis() }
.reduce { acc, d -> acc + d }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! I think we can simplify this with this.sumByDouble { it.elapsedTime.millis() }. Let me know what do you think. Thanks!

@jan-goral jan-goral force-pushed the 557-incomplete-time-info branch from 2b3ab46 to 630f2b7 Compare March 30, 2020 08:49
@jan-goral jan-goral merged commit b5a56fa into master Mar 30, 2020
@jan-goral jan-goral deleted the 557-incomplete-time-info branch March 30, 2020 08:58
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.

Time info incomplete when using orchestrator
4 participants