internal/scorecard/alpha: add parallelism support#3434
internal/scorecard/alpha: add parallelism support#3434joelanford merged 2 commits intooperator-framework:masterfrom
Conversation
jmccormick2001
left a comment
jmccormick2001
left a comment
There was a problem hiding this comment.
looks good to me, tested out with both parallel and non-parallel
| Entrypoint: test.Entrypoint, | ||
| Labels: test.Labels, | ||
| } | ||
| output.Items = append(output.Items, item) |
There was a problem hiding this comment.
usually nor happy with for inside to another for, however, I think it will not have too many items so 👍
There was a problem hiding this comment.
That's the only way this works 😄
This isn't an O(n^2) problem like it seems like you're referring to. Stages are just groups of tests. This function is (generally) O(number of tests) regardless of whether stages are introduced or not.
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| default: | ||
| return r.TestStatus, r.Error |
There was a problem hiding this comment.
should we not keep the default?
There was a problem hiding this comment.
No because default in a select statement is used when none of the channels have available items. If we left default in, it would "win" and return immediately when what we really want is to wait for the timer or the context to expire.
camilamacedo86
left a comment
camilamacedo86
left a comment
There was a problem hiding this comment.
Hi @joelanford,
just 1 nit; https://github.com/operator-framework/operator-sdk/pull/3434/files#r455418032
Otherwise, it shows /lgtm /approve for me.
| Image: test.Image, | ||
| Entrypoint: test.Entrypoint, | ||
| Labels: test.Labels, | ||
| for _, stage := range o.Config.Stages { |
There was a problem hiding this comment.
Is it possible to have duplicate tests across stages? I suspect this wouldn't be common if so.
There was a problem hiding this comment.
As far as I know, there is nothing that would prevent it. Scorecard would happily run it twice and report the results twice. It would probably also handle duplicate tests in a single stage the same way.
There was a problem hiding this comment.
Running duplicates is obviously fine. Test names listed may be duplicates though, which looks a little weird.
| @@ -53,6 +54,7 @@ type PodTestRunner struct { | |||
| } | |||
|
|
|||
| type FakeTestRunner struct { | |||
There was a problem hiding this comment.
For a follow-up: move this to run_test.go.
estroz
left a comment
estroz
left a comment
There was a problem hiding this comment.
A bunch of nits, otherwise
/lgtm
5 participants
Description of the change:
Re-opening of #3420
Add support for stages and parallelism in scorecard
Motivation for the change:
Give users more flexbility in scheduling their tests and having them complete faster when they can be run in parallel.
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs