-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
roachtest: refactor github issue posting and expose actual vm args for reporting #87304
roachtest: refactor github issue posting and expose actual vm args for reporting #87304
Conversation
87b6d75
to
e69adb3
Compare
e69adb3
to
3d89d17
Compare
3d89d17
to
b29f967
Compare
0828b65
to
ad6f21e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, thanks for all the refactorings!
A couple of comments/notes:
- Could you add more detailed commit descriptions to your commits? I feel like they (especially the first one) could use some context for the change.
- Now that we have access to the
vm.CreateOpts
, another parameter we can include in the reports is if we are using local SSD
Something I've been thinking about lately that relates to this PR is that it would be nice if we had an API where we could aggregate relevant state during the execution of a test, and that state was reported in case the test failed. Something like t.SetParam("seed", seed)
. The rationale is that tests currently log this type of granular data and they are often necessary to understand what a test did or to help reproduce it (such as a pseudo-random seed, as an example). So the roachtest framework could add its own parameters (prefixed with ROACHTEST_
as they are today), but we would also expose the ability for test writers to add test-specific context. I feel like this would be helpful especially in cases where we have multiple failures in a single GitHub issue; it allows us to get a quick glimpse of these parameters in the issue report itself, perhaps even surfacing patterns.
We don't need to adopt something like this for this PR, just wanted to hear if anyone has thoughts on this.
Reviewed 1 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)
pkg/cmd/roachtest/github.go
line 27 at r2 (raw file):
) type githubIssues interface {
We don't need the interface in this case, as it doesn't add much value. If we want to indicate to outside callers which methods on githubIssues
they should call, we can use upper and lowercase method names, to mimic public and private methods.
pkg/cmd/roachtest/github.go
line 41 at r2 (raw file):
cluster *clusterImpl vmCreateOpts *vm.CreateOpts issuePoster func(ctx context.Context, formatter issues.IssueFormatter, req issues.PostRequest) error
The signature here was taken from issues.Post
, I believe. It creates an unnecessary coupling and as far as I can tell, we are only injecting this function like this in order to test maybePost
. Suggestion: what if instead this struct had a method buildRequest
that created the issues.PostRequest
that you want to test. You could then have unit tests for shouldPost
and buildRequest
, making maybePost
dumb enough to not require unit tests (just shouldPost
+ issues.Post
).
pkg/cmd/roachtest/github.go
line 52 at r2 (raw file):
t.Spec().(*registry.TestSpec).Cluster.NodeCount > 0 } func roachtestPrefix(p string) string {
Don't you need a newline here? I thought the formatter would add that automatically.
pkg/cmd/roachtest/github_test.go
line 1 at r2 (raw file):
// Copyright 2018 The Cockroach Authors.
We should use 2022
here as this is a new file.
pkg/cmd/roachtest/github_test.go
line 35 at r2 (raw file):
data := []byte(teams) err := os.WriteFile("TEAMS.yaml", data, 0644)
Running this test will lead to a TEAMS.yaml
being created, which is not ideal. We could delete the file after the test.
Or, perhaps a better approach is to use loadTeams()
instead of DefaultLoadTeams()
. The former is already overwritten during tests. You could even specify your own version here if necessary.
pkg/cmd/roachtest/github_test.go
line 103 at r2 (raw file):
testSpec := ®istry.TestSpec{ Name: "githubPost", Owner: "unowned",
Can be replaced by OwnerUnitTest
.
pkg/cmd/roachtest/github_test.go
line 104 at r2 (raw file):
Name: "githubPost", Owner: "unowned", Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {},
Doesn't need to be specified.
pkg/cmd/roachtest/github_test.go
line 127 at r2 (raw file):
cluster: testClusterImpl, l: nilLogger(), issuePoster: func(ctx context.Context, formatter issues.IssueFormatter, req issues.PostRequest) error {
All of the assertions here could be simplified if we use github.com/stretchr/testify/require
(e.g., require.Equal(t, a, b)
pkg/cmd/roachtest/test_runner.go
line 1135 at r1 (raw file):
// TODO(tbg): nothing in this method should have the `t`; they should have a `Logger` only. // Maybe done
Seems like the TODO has been addressed, right? We can remove the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're absolutely right. I will absolutely amend the commit messages.
It makes sense to me being able to specify a seed for probabilistic branches in the tests, which also is not a big change to make.
Another thing worth thinking about is as we improve reporting and information on issues, are they being presented in the best way possible. That's not to say pushing these values via additional parameters
is not adequate, but the more information we pass, the more information an engineer has to parse.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @smg260, @srosenberg, and @tbg)
pkg/cmd/roachtest/github.go
line 27 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
We don't need the interface in this case, as it doesn't add much value. If we want to indicate to outside callers which methods on
githubIssues
they should call, we can use upper and lowercase method names, to mimic public and private methods.
Does having the interface makes it easier to ultimately increase coverage of runTest
in test_runner.go
? My immediate thought was to abstract it since it is making an external call - though I can see it makes little sense as private functions.
pkg/cmd/roachtest/github.go
line 41 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
The signature here was taken from
issues.Post
, I believe. It creates an unnecessary coupling and as far as I can tell, we are only injecting this function like this in order to testmaybePost
. Suggestion: what if instead this struct had a methodbuildRequest
that created theissues.PostRequest
that you want to test. You could then have unit tests forshouldPost
andbuildRequest
, makingmaybePost
dumb enough to not require unit tests (justshouldPost
+issues.Post
).
Agreed, and it was my initial approach as well, but wanted to keep this refactor as close to the original as possible. I will make this change.
pkg/cmd/roachtest/github.go
line 52 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Don't you need a newline here? I thought the formatter would add that automatically.
Formatter was being pretty aggressive everywhere - not sure why it wasn't here.
pkg/cmd/roachtest/github_test.go
line 1 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
We should use
2022
here as this is a new file.
Will do, thanks
pkg/cmd/roachtest/github_test.go
line 35 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Running this test will lead to a
TEAMS.yaml
being created, which is not ideal. We could delete the file after the test.Or, perhaps a better approach is to use
loadTeams()
instead ofDefaultLoadTeams()
. The former is already overwritten during tests. You could even specify your own version here if necessary.
I don't think there's anything inherently bad about creating files in a unit test and afaik, they are run in a sandboxed environment.
LoadTeams
would require another argument to be passed in (io.Reader
) by the caller.
pkg/cmd/roachtest/github_test.go
line 103 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Can be replaced by
OwnerUnitTest
.
Will do
pkg/cmd/roachtest/github_test.go
line 104 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Doesn't need to be specified.
you're right - I was initially looking at test_runner.go
which explicitly checks that Run
is specified
pkg/cmd/roachtest/github_test.go
line 127 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
All of the assertions here could be simplified if we use
github.com/stretchr/testify/require
(e.g.,require.Equal(t, a, b)
Will do - that's much better - thanks.
pkg/cmd/roachtest/test_runner.go
line 1135 at r1 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Seems like the TODO has been addressed, right? We can remove the comment.
Yep - wasn't sure if there was more to it. Looks straightforward but will run by @tbg . This was a boy scout change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @smg260, @srosenberg, and @tbg)
pkg/cmd/roachtest/github.go
line 41 at r2 (raw file):
Previously, smg260 (Miral Gadani) wrote…
Agreed, and it was my initial approach as well, but wanted to keep this refactor as close to the original as possible. I will make this change.
On 2nd thought, even if I dumb down the maybePost, it is still a method which has branches in it. Regardless of its simplicity, I feel that if branches can be covered without compromising the code too much, they should. Injecting the function enables this.
Regarding coupling, is the code really any more coupled that calling issues.Post directly
? By its nature, we are coupled to the API signature already.
I think there may be a compromise here somewhere. Let me work it.
Sorry for the notification to review everyone. Looks like it was caused by an erroneous rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Reviewed 3 of 7 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @smg260, and @srosenberg)
pkg/cmd/roachtest/github_test.go
line 197 at r5 (raw file):
require.Equal(t, v, req.ExtraParams[k]) } }
Is this needed? I thought the require
call above would be sufficient.
pkg/cmd/roachtest/github_test.go
line 204 at r5 (raw file):
require.Equal(t, "release-blocker", lastLabel) } else { require.Equal(t, "O-roachtest", lastLabel)
Nit: from the test's perspective, all we care about is that we have the O-roachtest
label, and -- conditionally -- the release-blocker
label. So we could change the test to look for the presence of the labels, rather than their position in the array to make it less tied to the implementation.
d7427e5
to
839b350
Compare
…test. No previous coverage exists for posting issues to github. This change moves the relevant code into a separate source file with associated test. External functions dealing with actual issue posting to github, and loading the TEAMS.yaml are now injected to facilitate easier testing. Release justification: test-only change Release note: none
839b350
to
9d2a389
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @renatolabs, and @srosenberg)
pkg/cmd/roachtest/github.go
line 27 at r2 (raw file):
Previously, smg260 (Miral Gadani) wrote…
Does having the interface makes it easier to ultimately increase coverage of
runTest
intest_runner.go
? My immediate thought was to abstract it since it is making an external call - though I can see it makes little sense as private functions.
Done
pkg/cmd/roachtest/github_test.go
line 197 at r5 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Is this needed? I thought the
require
call above would be sufficient.
🤦 Fixed.
pkg/cmd/roachtest/github_test.go
line 204 at r5 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Nit: from the test's perspective, all we care about is that we have the
O-roachtest
label, and -- conditionally -- therelease-blocker
label. So we could change the test to look for the presence of the labels, rather than their position in the array to make it less tied to the implementation.
Yes you're right. I was trying to avoid iterating over the slice but we have an existing utility. Fixed
bors r=renatolabs |
Build failed (retrying...): |
Build succeeded: |
blathers backport 22.2 |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from ac3ba1d to blathers/backport-release-22.2-87304: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
The change is split into 2 commits.
These commits can be squashed but are separate at the moment for convenience.
Resolves: #81846
Release justification: test-only change
Release note: none