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

*: TestNoLinkForbidden tests broken #74119

Closed
RaduBerinde opened this issue Dec 21, 2021 · 3 comments · Fixed by #100952
Closed

*: TestNoLinkForbidden tests broken #74119

RaduBerinde opened this issue Dec 21, 2021 · 3 comments · Fixed by #100952
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@RaduBerinde
Copy link
Member

RaduBerinde commented Dec 21, 2021

We have various TestNoLinkForbidden tests that are supposed to prevent certain packages being linked into specific binaries. For example: https://github.com/cockroachdb/cockroach/blob/master/pkg/cli/flags_test.go#L58

These don't work anymore. Apparently testing is linked into the main cockroach binary, along with other packages that are supposed to be test-only.

Jira issue: CRDB-11925

@RaduBerinde RaduBerinde added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Dec 21, 2021
@RaduBerinde
Copy link
Member Author

Fixing the dependencies is tracked by #74120.

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Dec 21, 2021
The `sql/randgen` package creates a lot of global datums, some of
which use geospatial and require the loading of geospatial data. This
package is meant for testing and should not be part of the cockroach
binary.

This change removes the non-testing uses of randgen.

Tested via `go list -deps ./pkg/cmd/cockroach`. Note that the updated
test is ineffectual for now (tracked by cockroachdb#74119).

Informs cockroachdb#74120.

Release note: None
craig bot pushed a commit that referenced this issue Dec 21, 2021
73941: roachtest: make crdb crash on span-use-after-Finish r=andreimatei a=andreimatei

This patch makes roachtest pass an env var to crdb asking it to panic on
mis-use of tracing spans. I've been battling such bugs, which become
more problematic as I'm trying to introduce span reuse. In production
we'll probably continue tolerating such bugs for the time being, but I
want tests to yell. Unit tests are already running with this
use-after-Finish detection, and so far so good. I've done a manual run
of all the roachtests in this configuration and nothing crashed, so I
don't expect a tragedy.

Release note: None

74109: kv: rename gcQueue to mvccGCQueue r=tbg a=nvanbenschoten

This commit renames the "GC queue" to the "MVCC GC queue" (which GC's old MVCC
versions) to avoid confusion with the "replica GC queue" (which GC's abandoned
replicas). We've already been using this terminology in various other contexts
to avoid confusion, so this refactor updates the code to reflect this naming.

This comes in response to #73838, which found a bug that had survived for three
years and was a direct consequence of this ambiguous naming.

The commit doesn't go quite as far as renaming the `pkg/kv/kvserver/gc` package,
but that could be a follow-up to this commit.

74126: geo: move projection data to embedded compressed file r=RaduBerinde a=RaduBerinde

The geoprojbase package embeds projection info as constants, leading
to a 6MB code file. Large code files are undesirable especially from
the perspective of static analysis tools, IDEs, etc.

This change moves the projections data to an embedded json.gz file. We
define the schema of this file in a new `embeddedproj` subpackage. The
data is loaded lazily.

The data file was obtained by modifying the existing constants to fill
out an `embeddedproj.Data`:
https://github.com/RaduBerinde/cockroach/blob/geospatial-proj-data/pkg/geo/geoprojbase/embeddedproj/data_test.go

The `generate-spatial-ref-sys` command is also updated to generate
this file from the `.csv`.

The `make buildshort` binary size is decreased by ~7MB.

Fixes #63969.

Release note: None

74128: cockroach: don't import randgen in binary r=RaduBerinde a=RaduBerinde

The `sql/randgen` package creates a lot of global datums, some of
which use geospatial and require the loading of geospatial data. This
package is meant for testing and should not be part of the cockroach
binary.

This change removes the non-testing uses of randgen.

Tested via `go list -deps ./pkg/cmd/cockroach`. Note that the updated
test is ineffectual for now (tracked by #74119).

Informs #74120.

Release note: None

74159: sql: default index recommendations to be off for logic tests  r=nehageorge a=nehageorge

**sql: refactor GlobalDefault for session variables**

This commit refactors pkg/sql/vars.go to use globalFalse and
globalTrue as the setting GlobalDefault where possible.

Release note: None

**sql: default index recommendations to be off for logic tests**

This commit configures index recommendations to be off for logic tests.
This is to avoid flaky tests, as the index recommendation output can
vary depending on the best plan chosen by the optimizer.

Fixes: #74069.

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Neha George <neha.george@cockroachlabs.com>
gustasva pushed a commit to gustasva/cockroach that referenced this issue Jan 4, 2022
The `sql/randgen` package creates a lot of global datums, some of
which use geospatial and require the loading of geospatial data. This
package is meant for testing and should not be part of the cockroach
binary.

This change removes the non-testing uses of randgen.

Tested via `go list -deps ./pkg/cmd/cockroach`. Note that the updated
test is ineffectual for now (tracked by cockroachdb#74119).

Informs cockroachdb#74120.

Release note: None
@ajwerner
Copy link
Contributor

ajwerner commented Feb 8, 2022

#64450 was my attempt from April 2021 to fix the test. The problem was our use of the go/build library IIRC. I think there are some bazel problems. https://github.com/cockroachdb/cockroach/pull/64450/files#diff-be4cad1a051288c41f125d8a421a7c7e41f93027f82abd1baa73ef3296e6480e worked at the time.

Many of our import rules have rotted.

@rickystewart
Copy link
Collaborator

See also #79299.

craig bot pushed a commit that referenced this issue Apr 10, 2023
100813: streamingccl: deflake TestRandomClientGeneration r=adityamaru a=msbutler

This patch fixes 4 bugs in TestRandomClientGeneration that were responsible for the persistent flakiness and lack of coverage in this test:
- the randomeStreamClient no longer instantiates keys with a table prefix that collides with the job info table prefix. This collision was the original cause of the flakes reported in #99343.
- getPartitionSpanToTableId() now generates a correct map from source partition key space to table Id. Previously, the key spans in the map didn't contain keys that mapped to anything logical in the cockroach key space.
- assertKVs() now checks for keys in the destination tenant keyspace.
- assertKVs() now actually asserts that kvs were found. Before, the assertion could pass if no keys were actually checked, which has been happening for months and allowed the bugs above to infest this test.

Fixes #99343

Release note: None

100952: cli: trash `TestNoLinkForbidden` r=rail a=rickystewart

This test does not work:
1. The test [has been broken](#74119) for years.
2. The test is not sensible in the Bazel world anyway, and under remote execution the test fails with an error like the following:

```
 build.go:59: go/build: go list github.com/cockroachdb/cockroach/pkg/cmd/cockroach: fork/exec GOROOT/bin/go: no such file or directory
```

The bug to replace this test with working functionality based on Bazel is #81526.

Epic: CRDB-17165
Release note: None
Closes #74119.

100965: sql: link issue to unimplemented mutations in udfs r=mgartner,rytaft a=rharding6373

Links an issue to the unimplemented errors for mutations in UDFs.

Epic: None
Informs: #87289
Fixes: #99715

Release note: None

Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Co-authored-by: Ricky Stewart <rickybstewart@gmail.com>
Co-authored-by: rharding6373 <rharding6373@users.noreply.github.com>
@craig craig bot closed this as completed in 3522c98 Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants