Print retires & display additional info #779
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
jan-goral
force-pushed
the
717-print-retries
branch
from
1ce78ca to
760fda1
Compare
|
I have added 2 additional styles for displaying single & multi line shortened output. On consoles which not honors ansi codes multi line output will not be display correctly so single line output can be used. Multi line output: Single line output (it's port of gcloud MonitorTestMatrixProgress function): In both cases old output is overwritten when status has changed I will add new flank option |
jan-goral
force-pushed
the
717-print-retries
branch
from
c3d2165 to
6ce9105
Compare
Codecov Report
@@ Coverage Diff @@
## master #779 +/- ##
============================================
+ Coverage 78.50% 79.04% +0.53%
- Complexity 692 701 +9
============================================
Files 133 136 +3
Lines 2857 2921 +64
Branches 411 419 +8
============================================
+ Hits 2243 2309 +66
+ Misses 355 344 -11
- Partials 259 268 +9 |
jan-goral
requested review from
pawelpasterz,
bootstraponline and
adamfilipow92
and removed request for
pawelpasterz
jan-goral
force-pushed
the
717-print-retries
branch
from
a79a83b to
735b50e
Compare
jan-goral
force-pushed
the
717-print-retries
branch
from
735b50e to
d19ab8c
Compare
This was
linked to
issues
May 15, 2020
jan-goral
force-pushed
the
717-print-retries
branch
from
6e23e9a to
bc58267
Compare
pawelpasterz
previously approved these changes
May 15, 2020
There was a problem hiding this comment.
Amazing and great work! I really like how logs look like now. That's huge new feature in flank.
General comment to my comments ( 😃 ) none is change request. Some are more about personal preferences and some (NPE related) might be too 'safe'. I'd love to get some responses from you and I am really curious about them.
Again,I like the code, nice!
| @@ -47,3 +49,9 @@ interface IArgs { | |||
| val AVAILABLE_SHARD_COUNT_RANGE = 1..50 | |||
| } | |||
| } | |||
|
|
|||
| fun IArgs.outputStyle() = outputStyle ?: let { | |||
There was a problem hiding this comment.
I think let is redundant here
|
|
||
| fun IArgs.outputStyle() = outputStyle ?: let { | ||
| if (flakyTestAttempts > 0 || (!disableSharding && maxTestShards > 0)) | ||
| OutputStyle.Multi else |
There was a problem hiding this comment.
Yeah I know I'm again and again about the same but really don't like formatting like this 😄 But as usual -- matter of personal preferences so don't bother my comment
| stopWatch.start() | ||
| } | ||
|
|
||
| private val cache = testMatricesIds.map { it to MatrixState.PENDING }.toMap().toMutableMap() |
There was a problem hiding this comment.
I think we can simplify this with .associateWith
There was a problem hiding this comment.
Nice, I missed this, thx.
| delay(5_000) | ||
| refreshedMatrix = GcTestMatrix.refresh(matrixId, args.project) | ||
| val matrix = GcTestMatrix.refresh(testMatrixId, projectId) | ||
| emit(matrix) |
There was a problem hiding this comment.
I am wondering if we should not check matrix.testMatrixId for null. TestMatrix is java object and according to docs
/**
* Output only. Unique id set by the service.
* @return value or {@code null} for none
*/
public java.lang.String getTestMatrixId() {
return testMatrixId;
}
I know it doesn't make sense to have null here and there is no reason why FTL return would want null here. We have some experience with FTL API and sometimes it returns errors or broken objects. (even recently there was a case when FTL returned test execution without time and that caused flank to blow).
If null would appear here we could have NPE in L35 matrices + (next.testMatrixId to next) or TestMatrixStatusPrinter#invoke -> cache[matrix.testMatrixId] = matrix.state.
If such case would occur we can use as fallback (and assign) testMatrixId passed as method's argument.
I don't know, maybe I'm overreacting :) Let me know what you think
There was a problem hiding this comment.
Maybe you have right but is should be done by GcTestMatrix.refresh, not here
| @@ -20,7 +21,7 @@ suspend fun newTestRun(args: IArgs) { | |||
| val (matrixMap, testShardChunks) = cancelTestsOnTimeout(args.project) { runTests(args) } | |||
|
|
|||
| if (!args.async) { | |||
| cancelTestsOnTimeout(args.project, matrixMap.map) { pollMatrices(matrixMap, args) } | |||
| cancelTestsOnTimeout(args.project, matrixMap.map) { pollMatrices(matrixMap.map.keys, args).update(matrixMap) } | |||
There was a problem hiding this comment.
How about moving update to the end of pollMatrices (in PollMatrices.kt) method chain? Same for RefreshLastRun.kt. Then it would be extension function available inside this file.
Let me know what you think
There was a problem hiding this comment.
The idea was that pollMatrices shoud not be aware of SavedMatrix. SavedMatrix looks like old school java class. I dont like black boxes. I have plans to get rid of it in the future.
| val time = stopWatch.check(alignSeconds = true) | ||
| printExecutionStatusList(time, matrix.testExecutions) | ||
| cache[matrix.testMatrixId] = matrix.state | ||
| if (allMatricesCompleted) printTestMatrixStatusList(time) |
There was a problem hiding this comment.
I am not sure if it's best solution to print all matrices status when all of them are finished. There might be one test pair with very few tests and could end couple of minutes (or more) before the rest. In such case user should be notified immediately.
On the other hand I am not sure if this is doable in a way to preserve current logs structure (matrix final status at the end of all logs) which is great...
Let me know what you think and if this is possible to achieve, thanks!
There was a problem hiding this comment.
I will think about that but pls, not in this PR.
| private fun TestExecution.formatName(args: IArgs): String { | ||
| val matrixExecutionId = id.split("_") | ||
| val matrixId = matrixExecutionId.first() | ||
| val executionId = matrixExecutionId.takeIf { args.flakyTestAttempts > 0 }?.getOrNull(1)?.let { " $it" } ?: "" |
There was a problem hiding this comment.
I am wondering if executionId is that much needed (in that form). I understand intention to explicitly indicate if it's re-run or not. As user I would expect here info that is similar to FTL console.
But hey, this PR is big enough without that feature. And I don't know if it's easy easy to achieve it. I think this is something we can try to improve in the future.
Let me know what you think.
There was a problem hiding this comment.
I agree, in the future. ;)
| } | ||
|
|
||
| private fun TestExecution.executionStatus() = ExecutionStatus( | ||
| state = state, |
| } | ||
|
|
||
| private fun TestExecution.formatName(args: IArgs): String { | ||
| val matrixExecutionId = id.split("_") |
| } | ||
| } | ||
|
|
||
| private val listExecutionStatusView: (ExecutionStatus.Change) -> List<ExecutionStatus.View> |
There was a problem hiding this comment.
How about extension property here instead? Something like (just proposal, not change request :) ):
private val ExecutionStatus.Change.views: List<ExecutionStatus.View>
get() = current.progress
.minus(previous.progress)
.map { ExecutionStatus.View(time, name, it) }
.let { list ->
if (current.error != previous.error && current.error != null)
list + ExecutionStatus.View(time, name, "Error: ${current.error}")
else list
}
.let { list ->
if (current.state != previous.state)
list + ExecutionStatus.View(time, name, current.state)
else list
}
Let me know what you think?
There was a problem hiding this comment.
It doesn't make much difference, but I can change it.
There was a problem hiding this comment.
IMO it's more clear when I see sth like
change.views.takeLast
but I don't want to force anything.
pawelpasterz
previously approved these changes
May 16, 2020
runningcode
reviewed
May 17, 2020
Co-authored-by: Nelson Osacky <[email protected]>
pawelpasterz
approved these changes
May 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #717 #681 #738
Checklist