roachtest: change the name of the jepsen top level tests #30900
Conversation
|
This change is |
pkg/cmd/roachtest/jepsen.go
Outdated
| @@ -270,7 +277,7 @@ func registerJepsen(r *registry) { | |||
|
|
|||
| for i := range groups { | |||
| spec := testSpec{ | |||
| Name: fmt.Sprintf("jepsen/%d", i+1), | |||
| Name: fmt.Sprintf("jepsen-group-%d", i+1), | |||
There was a problem hiding this comment.
Maybe just change the separator from / to -? These test names are already verbose so adding -group might be annoying (and IIRC there's a length limit somewhere but I'm not sure what it is)
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/cmd/roachtest/jepsen.go, line 280 at r1 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
Maybe just change the separator from
/to-? These test names are already verbose so adding-groupmight be annoying (and IIRC there's a length limit somewhere but I'm not sure what it is)
Was this causing a problem? Various other tests include / in the name. I wouldn't want to outlaw that without reason.
Replace slashes; they were confusing as that's what is used for subtests in different places. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/jepsen.go, line 280 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Was this causing a problem? Various other tests include
/in the name. I wouldn't want to outlaw that without reason.
changed to jepsen-g1. I want something besides a simple number because a number also looks too much like a run number.
Peter, it wasn't causing a problem, but it was getting me confused. I'm not outlawing anything, but I do think that, particularly for tests with subtests, slashes should be discouraged. Otherwise, are jepsen/1/foo and jepsen/2/bar sharing a cluster? Could be, could not be.
andreimatei
force-pushed
the
jepsen-name
branch
from
c0f587c
to
f07fdf7
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/jepsen.go, line 280 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
changed to
jepsen-g1. I want something besides a simple number because a number also looks too much like a run number.Peter, it wasn't causing a problem, but it was getting me confused. I'm not outlawing anything, but I do think that, particularly for tests with subtests, slashes should be discouraged. Otherwise, are
jepsen/1/fooandjepsen/2/barsharing a cluster? Could be, could not be.
acceptance/{bank,cli,gossip}/* fall afoul of your rule here. If you want to enforce some requirement on naming, I'd much prefer if it is done uniformly and enforced in some manner so we don't see inconsistencies. I don't particularly see a need for this rule, but I also don't feel strongly about it either way.
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/jepsen.go, line 280 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
acceptance/{bank,cli,gossip}/*fall afoul of your rule here. If you want to enforce some requirement on naming, I'd much prefer if it is done uniformly and enforced in some manner so we don't see inconsistencies. I don't particularly see a need for this rule, but I also don't feel strongly about it either way.
Again, I wasn't looking to introduce any new rule.
The names for the jepsen groups were particularly bad (jepsen/1/foo) - because the number can also be confused with a run counter (for repeated runs) that I'm trying to introduce. Think of renaming those as running into a log message that could be improved and improving it.
I've added a commit to rename the tests you mentioned (although the acceptance subtests in question are already subtests and they themselves don't have their own subtests, and so they don't technically "fall afoul" of what I've said).
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/jepsen.go, line 280 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Again, I wasn't looking to introduce any new rule.
The names for the jepsen groups were particularly bad (jepsen/1/foo) - because the number can also be confused with a run counter (for repeated runs) that I'm trying to introduce. Think of renaming those as running into a log message that could be improved and improving it.I've added a commit to rename the tests you mentioned (although the acceptance subtests in question are already subtests and they themselves don't have their own subtests, and so they don't technically "fall afoul" of what I've said).
You've introduced consistency, but not enforcement. One side-effect of this change is that we may get a spurt of new issues filed due to these renamings. Maybe. I'm not sure how the issue search works and if it will consider / differently than -. I'll let you take care of the fallout. 😄
andreimatei
force-pushed
the
jepsen-name
branch
from
baf86b1
to
f07fdf7
Compare
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/cmd/roachtest/jepsen.go, line 280 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
You've introduced consistency, but not enforcement. One side-effect of this change is that we may get a spurt of new issues filed due to these renamings. Maybe. I'm not sure how the issue search works and if it will consider
/differently than-. I'll let you take care of the fallout. 😄
Yeah, actually, I've reverted the last commit. My heart was not in it; I don't think it's worth changing those acceptance subtests.
I'm going ahead with the Jepsen rename, though. I care mostly about top-level test names (as that's where top-level vs subtests carries semantics), and also I dislike the Jepsen numbers too much to leave them alone :)
Merging, but I'll continue the discussion if you want to.
Build succeeded |
|
I belatedly realized that |
|
Addressing in #30925
…On Wed, Oct 3, 2018 at 10:31 AM Ben Darnell ***@***.***> wrote:
I belatedly realized that g is confusing here because one of the jepsen
tests is named g2.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#30900 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXBcdsrS3tg0GPuWJy4-_rj5dYaXcT7ks5uhMpOgaJpZM4XE2sx>
.
|
Replace slashes; they were confusing as that's what is used for subtests
in different places.
Release note: None