Skip to content
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

util: support specifying catalog of metamorphic constant values #114039

Open
jbowens opened this issue Nov 8, 2023 · 10 comments
Open

util: support specifying catalog of metamorphic constant values #114039

jbowens opened this issue Nov 8, 2023 · 10 comments
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team

Comments

@jbowens
Copy link
Collaborator

jbowens commented Nov 8, 2023

The use of metamorphic constants helps us expand test coverage through testing varied code paths. But when we reproduce a failure, changing metamorphic constants can make it more challenging to reproduce a failure. It would be a large quality-of-life improvement if test failures wrote out a catalog of metamorphic constants and their values, and it was easy to re-run a test loading constants from that catalog.

Jira issue: CRDB-33313

@jbowens jbowens added A-testing Testing tools and infrastructure T-testeng TestEng Team labels Nov 8, 2023
Copy link

blathers-crl bot commented Nov 8, 2023

cc @cockroachdb/test-eng

Copy link

blathers-crl bot commented Nov 8, 2023

Hi @jbowens, please add a C-ategory label to your issue. Check out the label system docs.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@DarrylWong
Copy link
Contributor

I agree it would certainly be nice for the set of metamorphic constants used to be more obvious. Maybe in it's own log file that's very clearly named? And then a print statement in test.log pointing the user towards that file(s)?

In the meanwhile, you can still find these constants in cockroach.stderr.log file.

it was easy to re-run a test loading constants from that catalog

#113301 will add a helpful hint on how to rerun tests with the same seed.

@srosenberg srosenberg added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Nov 16, 2023
@DarrylWong
Copy link
Contributor

Clarified with Jackson that this is directed towards unit tests, but he mentioned that it would be nice for roachtests as well.

As for UX, he mentioned it'd be nice be able to re run a failing unit test with the same metamorphic constants minus the suspected constant. A suggested idea for how to do this would be a separate file from a test failure that can be easily modified to configure a future test run.

Similar to what's blocking #113164, this seems tricky because metamorphic constants are selected at init time. We'd likely have to rework the metamorphic constants framework first to allow for this.

@pav-kv
Copy link
Collaborator

pav-kv commented Dec 14, 2023

Can the whole list of constants be reproduced from a single rand seed? Print the seed, and add a parameter to set the seed?

@jbowens
Copy link
Collaborator Author

jbowens commented Dec 14, 2023

Can the whole list of constants be reproduced from a single rand seed? Print the seed, and add a parameter to set the seed?

Personally, I'd like the ability to re-run with all the same constants with explicitly set exceptions, which is tricky (impossible?) to do through a single seed.

@srosenberg
Copy link
Member

...all the same constants with explicitly set exceptions,

Could you please clarify what you mean by "explicitly set exceptions"? An example is best.

@jbowens
Copy link
Collaborator Author

jbowens commented Dec 14, 2023

When a particular metamorphic constant value causes a test failure, it's helpful to be able to run all the other metamorphic constants at the same settings but override the suspected constant. This issue was originally motivated by the issues linked from #114057. These tests failed readily with a particular metamorphic seed. Being able to re-run the tests with the same constants but overriding the metamorphism of storage.experimental.eventually_file_only_snapshots.enabled and kv.snapshot_receiver.excise.enabled would've helped the investigation.

@pav-kv
Copy link
Collaborator

pav-kv commented Dec 15, 2023

If the list of overrides is small, one can run with a seed + overrides listed in flags?

If you need to do the opposite (set a couple of variables as constants and randomize the rest), don't even need the seed: just run random generation + overrides.

@pav-kv
Copy link
Collaborator

pav-kv commented Dec 15, 2023

If you need to automate randomization of a couple of variables, and set the rest constant, then you need 3 flags:

  • The "global" seed. It defines all the values (but some of them will be overridden below).
  • The (short) list of variables that you are experimenting with (optionally with values that override what the seed above generated).
  • The (optional) seed to generate values just for this small list (and override what the global seed generated).

With these 3 flags, you can emulate all the use-cases:

  • Provide a full list of k=v values.
  • Generate a full list using the seed.
  • Generate most of the list using the seed, and override some values manually.
  • Generate most of the list using the seed, and generate the few values of interest using the other seed.

stevendanna added a commit to stevendanna/cockroach that referenced this issue Dec 10, 2024
This allows developers to override specific metamorphic variables:

   ./dev test ./pkg/foo -- --test_env COCKROACH_INTERNAL_METAMORPHIC_OVERRIDES=var1=val1,var2=val2

Metamorphic values not specified are still randomly chosen.

In a future commit, it would also be nice to be able to specify a file
that allows you to specify all values.

Epic: none
Informs cockroachdb#114039
stevendanna added a commit to stevendanna/cockroach that referenced this issue Dec 10, 2024
This allows developers to override specific metamorphic variables:

   ./dev test ./pkg/foo -- --test_env COCKROACH_INTERNAL_METAMORPHIC_OVERRIDES=var1=val1,var2=val2

Metamorphic values not specified are still randomly chosen.

In a future commit, it would also be nice to be able to specify a file
that allows you to specify all values.

Epic: none
Informs cockroachdb#114039
stevendanna added a commit to stevendanna/cockroach that referenced this issue Dec 11, 2024
This allows developers to override specific metamorphic variables:

   ./dev test ./pkg/foo -- --test_env COCKROACH_INTERNAL_METAMORPHIC_OVERRIDES=var1=val1,var2=val2

Metamorphic values not specified are still randomly chosen.

In a future commit, it would also be nice to be able to specify a file
that allows you to specify all values.

Epic: none
Informs cockroachdb#114039
craig bot pushed a commit that referenced this issue Dec 19, 2024
137080: metamorphic: add environment-based overrides r=asg0451 a=stevendanna

This allows developers to override specific metamorphic variables:

   ./dev test ./pkg/foo -- --test_env COCKROACH_INTERNAL_METAMORPHIC_OVERRIDES=var1=val1,var2=val2

Metamorphic values not specified are still randomly chosen.

In a future commit, it would also be nice to be able to specify a file that allows you to specify all values.

Epic: none
Informs #114039

137682: pgwire: rename ReadBuffer.GetString to GetUnsafeString r=fqazi a=fqazi

Previously, it was not clear that GetString would not copy data from the ReadBuffer. This could be problematic if the object was long lived, since the entire buffer would have been kept alive. To reduce risk, this patch renames GetString to GetUnsafeString. It also adds a GetSafeString method for cases where a copy is needed. The latter is adopted inside: parseClientProvidedSessionParameters

Informs: #137627
Release note: None

137699: kvflowcontrol: fix logging redaction for a few types r=pav-kv a=pav-kv

The strings inside `SafeFormat()` were not marked as safe, and caused the values being printed as redactable.

Part of #136526

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team
Projects
No open projects
Status: Triage
Development

No branches or pull requests

4 participants