cockroach
cockroach copied to clipboard
roachtest: make validation of workload voluntary
Similar to https://github.com/cockroachdb/cockroach/pull/115947 but now with also opting in many tests that were easily identified by grep as needing the old binary into validating it.
I'll leave the discussion on if we should add this up to the others since I have no strong opinion/limited insight.
But if we do go ahead with this change it'd be nice to force any tests that use t.DeprecatedWorkload()
to also set RequiresDeprecatedWorkload: true
. With how it is currently I could see a lot of people copy and pasting the c.Put()
without also copying the RequiresDeprecatedWorkload: true
Failing at compile time or before the test starts would be great, but not sure how feasible that is. At the minimum something like:
func (t *testImpl) DeprecatedWorkload() string {
if !t.spec.RequiresDeprecatedWorkload {
t.Fatalf("helpful error about how RequiresDeprecatedWorkload should be set")
}
return t.deprecatedWorkload
}
might be good?
Failing at compile time or before the test starts would be great
This seems like way more work than it would be worth for what is, to be clear, a deprecated field. I'm not even 100% convinced the fuss of opt-in validation is worth it for a deprecated feature; I only added it here if it helped folks be more okay with remove the requirement from the large majority of tests that are having it inflicted on them for no reason. I'm not super motivated to make it more complex / checked at runtime / etc to better handle new tests, since new tests shouldn't be using something marked as "deprecated" anyway.
This seems like way more work than it would be worth for what is, to be clear, a deprecated field.
It's not really a deprecated field despite having that moniker. The issue is that we have a number of workloads which are not part of the cockroach binary, for the simple reason of trying to contain its size. I don't see a quick resolution here, so the workload will likely have to remain undeprecated.