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

build: disable metamorphic constants when calling execgen #76309

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Feb 9, 2022

Commands such as ./dev test --verbose might previously produce
two log lines everytime it happens to have to call the genrule for
execgen targets:

INFO: From Executing genrule //pkg/sql/colexec/colexecwindow:gen-window-framer:
initialized metamorphic constant "span-reuse-rate" with value 10

This is because execgen has the tracing package in its dependency tree

> go list -f '{{ join .Deps "\n" }}' ./pkg/sql/colexec/execgen/ | grep tracing | head -1
github.com/cockroachdb/cockroach/pkg/util/tracing

and that package instantiates this metamorphic constant.

This PR sets COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when
calling the generator so we don't get metamorphic constants enabled.

Something like #75065 would give us more complete coverage of commands
that might be generating noise.

Release note: None

Commands such as `./dev test --verbose` might previously would produce
two log lines everytime it happens to have to call the genrule for
execgen targets:

```
INFO: From Executing genrule //pkg/sql/colexec/colexecwindow:gen-window-framer:
initialized metamorphic constant "span-reuse-rate" with value 10
```

This is because execgen has the tracing package in its dependency tree

```
> go list -f '{{ join .Deps "\n" }}' ./pkg/sql/colexec/execgen/ | grep tracing | head -1
github.com/cockroachdb/cockroach/pkg/util/tracing
```

and that package instantiates this metamorphic constant.

This PR sets COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when
calling the generator so we don't get metamorphic constants enabled.

Something like cockroachdb#75065 would give us more complete coverage of commands
that might be generating noise.

Release note: None
@stevendanna stevendanna requested a review from a team as a code owner February 9, 2022 17:24
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'd forgotten about #75065 but this is exactly where it came up. There was another instance in running partitioned package tests through bazel, when filtering for a single test by regex, but that wasn't so bad. I'll just close #75065 in favor of this.

@stevendanna
Copy link
Collaborator Author

bors r=irfansharif

@craig
Copy link
Contributor

craig bot commented Feb 10, 2022

Build succeeded:

@craig craig bot merged commit 9123557 into cockroachdb:master Feb 10, 2022
darinpp added a commit to darinpp/cockroach that referenced this pull request Mar 7, 2022
Previously cockroachdb#76309 added the ability to disable metamorphic constants
with env variable COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING which
disabled the metamorphic constants when set to true. For some tools
however (execgen, roachpb/gen) setting an evironment variable may not
be always possible. So this PR adds the ability to disable the
metamorphic testing through tag `metamorphic_disable`. It would seem
that similar thing could be achieved by using the crdb_test_off tag
instead. With bazel however this tag is ignored.

Fixes cockroachdb#76363

Release justification: Non-production code changes
Release note: None
craig bot pushed a commit that referenced this pull request Mar 7, 2022
77301: loqrecovery: fix json field name in replica info file r=erikgrinaker a=aliher1911

Previously protobuf used camelcase name for repeated field
for descriptor change. This was wrong as it was not parsed
correctly by marshaller.
This fix adds explicit name to proto specification to avoid
default name generation.

Release justification: This change is low risk as it fixes
a bug in new functionality.

Release note: None

Fixes #77282

77423: build: allow disabling metamorphic constants with tag r=darinpp a=darinpp

Previously #76309 added the ability to disable metamorphic constants
with env variable COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING which
disabled the metamorphic constants when set to true. For some tools
however (execgen, roachpb/gen) setting an evironment variable may not
be always possible. So this PR adds the ability to disable the
metamorphic testing through tag `metamorphic_disable`. It would seem
that similar thing could be achieved by using the crdb_test_off tag
instead. With bazel however this tag is ignored.

Fixes #76363

Release justification: Non-production code changes
Release note: None

Co-authored-by: Oleg Afanasyev <[email protected]>
Co-authored-by: Darin Peshev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants