dev: improve package argument handling #74808
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
postamar
force-pushed
the
dev-fix-73457
branch
2 times, most recently
from
886d4a7 to
ee83c04
Compare
|
Thanks for the review! bors r+ |
|
Build failed (retrying...): |
This commit adds syntax and existence checks to handle bad package arguments in a more robust way. Fixes cockroachdb#73457. Release note: None
postamar
force-pushed
the
dev-fix-73457
branch
from
ee83c04 to
a4da971
Compare
|
Canceled. |
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Feb 8, 2022
This commit groups a few changes to speed up dev and make it easier to test. 1. Update `dev bench` to run benchmarks using `bazel test` directly instead of first grepping for benchmarks, querying test targets, and only then invoking test binaries using `bazel run`. The latter approach was both slower and more difficult to test. `dev bench` now looks identical to the `dev test` implementation. 2. Simplify `dev test`; now that protobuf targets are directly buildable, we don't need any manual querying of build targets and filtering for only go_test ones -- we can more easy `bazel test` whatever was top-level package was specified. This reduces how much I/O needs to happen with the host environment which both speeds things up and makes it easier to test. a. It also allows us to partially revert cockroachdb#74808 while still retaining informative error messages about missing targets (bazel's ones out-of-the-box are pretty good). 3. Finally, introduce TestDataDriven and TestRecorderDriven. a. TestDataDriven makes use of datadriven to capture all operations executed by individual dev invocations. The testcases are defined under testdata/datadriven/*. DataDriven divvies up these files as subtests, so individual "files" are runnable through: dev test pkg/cmd/dev -f TestDataDrivenDriven/<fname> [--rewrite] OR go test ./pkg/cmd/dev -run TestDataDrivenDriven/<fname> [-rewrite] b. TestRecorderDriven makes use of datadriven/recorder to record (if --rewrite is specified) or play back (if --rewrite is omitted) all operations executed by individual dev invocations. The testcases are defined under testdata/recorderdriven/*; each test file corresponds to a captured recording found in testdata/recorderdriven/*.rec. dev test pkg/cmd/dev -f TestRecorderDriven/<fname> OR go test ./pkg/cmd/dev -run TestRecorderDriven/<fname> [-rewrite] Recordings are used to mock out "system" behavior. When --rewrite is specified, attempts to shell out to bazel or perform other OS operations (like creating, removing, symlinking filepaths) are intercepted and system responses are recorded for future playback. --- It's worth comparing TestDataDriven and TestRecorderDriven. 1. TestDataDriven is well suited for exercising flows that don't depend on reading external state in order to function (simply translating a `dev test <target>` to its corresponding bazel invocation for e.g.) All operations are run in "dry-run" mode when --rewrite is specified; all exec/os commands return successfully with no error + an empty response, and just the commands are recorded as test data. 2. TestRecorderDriven in contrast works better for flows that do depend on external state during execution (like reading the set of targets from a file for e.g., or hoisting files from a sandbox by searching through the file system directly). With these, when --rewrite is specified, we actually do shell out and record the data for future playback. We previously (cockroachdb#68514) ripped out the equivalent of TestRecorderDriven because it was difficult to use (large recordings, execution errors failing the test, etc.) -- issues this PR tries to address. There are some real limitations here though that may still make this approach untenable: When --rewrite is used for TestRecorderDriven, because it shells out, will: - takes time proportional to the actual dev invocations; - makes it difficult to run under bazel (through without --rewrite it works just fine); The few remaining tests for this recorder stuff are probably examples of dev doing too much, when it should instead push the logic down into bazel rules. As we continue doing so, we should re-evaluate whether this harness provides much value. Release note: None
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Feb 8, 2022
This commit groups a few changes to speed up dev and make it easier to test. 1. Update `dev bench` to run benchmarks using `bazel test` directly instead of first grepping for benchmarks, querying test targets, and only then invoking test binaries using `bazel run`. The latter approach was both slower and more difficult to test. `dev bench` now looks identical to the `dev test` implementation. 2. Simplify `dev test`; now that protobuf targets are directly buildable, we don't need any manual querying of build targets and filtering for only go_test ones -- we can more easy `bazel test` whatever was top-level package was specified. This reduces how much I/O needs to happen with the host environment which both speeds things up and makes it easier to test. a. It also allows us to partially revert cockroachdb#74808 while still retaining informative error messages about missing targets (bazel's ones out-of-the-box are pretty good). 3. Finally, introduce TestDataDriven and TestRecorderDriven. a. TestDataDriven makes use of datadriven to capture all operations executed by individual dev invocations. The testcases are defined under testdata/datadriven/*. DataDriven divvies up these files as subtests, so individual "files" are runnable through: dev test pkg/cmd/dev -f TestDataDrivenDriven/<fname> [--rewrite] OR go test ./pkg/cmd/dev -run TestDataDrivenDriven/<fname> [-rewrite] b. TestRecorderDriven makes use of datadriven/recorder to record (if --rewrite is specified) or play back (if --rewrite is omitted) all operations executed by individual dev invocations. The testcases are defined under testdata/recorderdriven/*; each test file corresponds to a captured recording found in testdata/recorderdriven/*.rec. dev test pkg/cmd/dev -f TestRecorderDriven/<fname> OR go test ./pkg/cmd/dev -run TestRecorderDriven/<fname> [-rewrite] Recordings are used to mock out "system" behavior. When --rewrite is specified, attempts to shell out to bazel or perform other OS operations (like creating, removing, symlinking filepaths) are intercepted and system responses are recorded for future playback. --- It's worth comparing TestDataDriven and TestRecorderDriven. 1. TestDataDriven is well suited for exercising flows that don't depend on reading external state in order to function (simply translating a `dev test <target>` to its corresponding bazel invocation for e.g.) All operations are run in "dry-run" mode when --rewrite is specified; all exec/os commands return successfully with no error + an empty response, and just the commands are recorded as test data. 2. TestRecorderDriven in contrast works better for flows that do depend on external state during execution (like reading the set of targets from a file for e.g., or hoisting files from a sandbox by searching through the file system directly). With these, when --rewrite is specified, we actually do shell out and record the data for future playback. We previously (cockroachdb#68514) ripped out the equivalent of TestRecorderDriven because it was difficult to use (large recordings, execution errors failing the test, etc.) -- issues this PR tries to address. There are some real limitations here though that may still make this approach untenable: When --rewrite is used for TestRecorderDriven, because it shells out, will: - takes time proportional to the actual dev invocations; - makes it difficult to run under bazel (through without --rewrite it works just fine); The few remaining tests for this recorder stuff are probably examples of dev doing too much, when it should instead push the logic down into bazel rules. As we continue doing so, we should re-evaluate whether this harness provides much value. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Feb 9, 2022
75499: sql: add constraint IDs and use them for comments r=fqazi a=fqazi This pull request will make the following changes: 1) Modify table descriptor logic to start allocating constraint IDs for each table constraint. 2) Add a migration to upgrade existing descriptors to have constraint IDs 3) Modify the comment updater / comment logic to references constraints by ConstraintID and update the obj_description builtin to look these up. 75888: kvstreamer: fix a bug with incorrect processing of empty scan responses r=yuzefovich a=yuzefovich **kvstreamer: fix a bug with incorrect processing of empty scan responses** Previously, the `Streamer` could get stuck indefinitely when a response for a Scan request came back empty - namely the response neither had no bytes inside nor ResumeSpan set (this is the case when there is no data in the key span to scan). This would lead to `GetResults` call being stuck thinking there are more responses to come back, and none would show up. This is now fixed by creating a Result with an empty Scan response inside of it. This approach makes it easier to support Scans that span multiple ranges and the last range has no data in it - we want to be able to set Complete field on such an empty Result. Since this was the only known bug with the `Streamer` implementation, the streamer is now used by default again. Fixes: #75708. Release note: None **kvstreamer: minor cleanup** This commit replaces the last buffered channel we had in the `Streamer` code in favor of a condition variable which simplifies the control flow a bit. Additionally, this commit refactors the code so that empty Get responses are not returned which allows us to clean up `TxnKVStreamer` a bit. Care has to be taken so that we still signal the client's goroutine waiting for results when an empty Get response comes back (similar to the bug fixed by the previous commit). Also, the tracking of the "number of outstanding requests" in `TxnKVStreamer` has been removed since it provided little value (and probably was broken anyway). Release note: None 76189: dev: test using datadriven r=irfansharif a=irfansharif This commit groups a few changes to speed up dev and make it easier to test. 1. Update `dev bench` to run benchmarks using `bazel test` directly instead of first grepping for benchmarks, querying test targets, and only then invoking test binaries using `bazel run`. The latter approach was both slower and more difficult to test. `dev bench` now looks identical to the `dev test` implementation. 2. Simplify `dev test`; now that protobuf targets are directly buildable, we don't need any manual querying of build targets and filtering for only go_test ones -- we can more easy `bazel test` whatever was top-level package was specified. This reduces how much I/O needs to happen with the host environment which both speeds things up and makes it easier to test. a. It also allows us to partially revert [#74808](#74808) while still retaining informative error messages about missing targets (bazel's ones out-of-the-box are pretty good). 3. Finally, introduce TestDataDriven and TestRecorderDriven. a. TestDataDriven makes use of datadriven to capture all operations executed by individual dev invocations. The testcases are defined under testdata/datadriven/*. DataDriven divvies up these files as subtests, so individual "files" are runnable through: dev test pkg/cmd/dev -f TestDataDrivenDriven/<fname> [--rewrite] OR go test ./pkg/cmd/dev -run TestDataDrivenDriven/<fname> [-rewrite] b. TestRecorderDriven makes use of datadriven/recorder to record (if --rewrite is specified) or play back (if --rewrite is omitted) all operations executed by individual dev invocations. The testcases are defined under testdata/recorderdriven/\*; each test file corresponds to a captured recording found in testdata/recorderdriven/\*.rec. dev test pkg/cmd/dev -f TestRecorderDriven/<fname> OR go test ./pkg/cmd/dev -run TestRecorderDriven/<fname> [-rewrite] Recordings are used to mock out "system" behavior. When --rewrite is specified, attempts to shell out to bazel or perform other OS operations (like creating, removing, symlinking filepaths) are intercepted and system responses are recorded for future playback. It's worth comparing TestDataDriven and TestRecorderDriven. a. TestDataDriven is well suited for exercising flows that don't depend on reading external state in order to function (simply translating a `dev test <target>` to its corresponding bazel invocation for e.g.) All operations are run in "dry-run" mode when --rewrite is specified; all exec/os commands return successfully with no error + an empty response, and just the commands are recorded as test data. b. TestRecorderDriven in contrast works better for flows that do depend on external state during execution (like reading the set of targets from a file for e.g., or hoisting files from a sandbox by searching through the file system directly). With these, when --rewrite is specified, we actually do shell out and record the data for future playback. We previously (#68514) ripped out the equivalent of TestRecorderDriven because it was difficult to use (large recordings, execution errors failing the test, etc.) -- issues this PR tries to address. There are some real limitations here though that may still make this approach untenable: When --rewrite is used for TestRecorderDriven, because it shells out, will: - takes time proportional to the actual dev invocations; - makes it difficult to run under bazel (through without --rewrite it works just fine); The few remaining tests for this recorder stuff are probably examples of dev doing too much, when it should instead push the logic down into bazel rules. As we continue doing so, we should re-evaluate whether this harness provides much value. Release note: None Co-authored-by: Faizan Qazi <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: irfan sharif <[email protected]>
RajivTS
pushed a commit
to RajivTS/cockroach
that referenced
this pull request
Mar 6, 2022
This commit groups a few changes to speed up dev and make it easier to test. 1. Update `dev bench` to run benchmarks using `bazel test` directly instead of first grepping for benchmarks, querying test targets, and only then invoking test binaries using `bazel run`. The latter approach was both slower and more difficult to test. `dev bench` now looks identical to the `dev test` implementation. 2. Simplify `dev test`; now that protobuf targets are directly buildable, we don't need any manual querying of build targets and filtering for only go_test ones -- we can more easy `bazel test` whatever was top-level package was specified. This reduces how much I/O needs to happen with the host environment which both speeds things up and makes it easier to test. a. It also allows us to partially revert cockroachdb#74808 while still retaining informative error messages about missing targets (bazel's ones out-of-the-box are pretty good). 3. Finally, introduce TestDataDriven and TestRecorderDriven. a. TestDataDriven makes use of datadriven to capture all operations executed by individual dev invocations. The testcases are defined under testdata/datadriven/*. DataDriven divvies up these files as subtests, so individual "files" are runnable through: dev test pkg/cmd/dev -f TestDataDrivenDriven/<fname> [--rewrite] OR go test ./pkg/cmd/dev -run TestDataDrivenDriven/<fname> [-rewrite] b. TestRecorderDriven makes use of datadriven/recorder to record (if --rewrite is specified) or play back (if --rewrite is omitted) all operations executed by individual dev invocations. The testcases are defined under testdata/recorderdriven/*; each test file corresponds to a captured recording found in testdata/recorderdriven/*.rec. dev test pkg/cmd/dev -f TestRecorderDriven/<fname> OR go test ./pkg/cmd/dev -run TestRecorderDriven/<fname> [-rewrite] Recordings are used to mock out "system" behavior. When --rewrite is specified, attempts to shell out to bazel or perform other OS operations (like creating, removing, symlinking filepaths) are intercepted and system responses are recorded for future playback. --- It's worth comparing TestDataDriven and TestRecorderDriven. 1. TestDataDriven is well suited for exercising flows that don't depend on reading external state in order to function (simply translating a `dev test <target>` to its corresponding bazel invocation for e.g.) All operations are run in "dry-run" mode when --rewrite is specified; all exec/os commands return successfully with no error + an empty response, and just the commands are recorded as test data. 2. TestRecorderDriven in contrast works better for flows that do depend on external state during execution (like reading the set of targets from a file for e.g., or hoisting files from a sandbox by searching through the file system directly). With these, when --rewrite is specified, we actually do shell out and record the data for future playback. We previously (cockroachdb#68514) ripped out the equivalent of TestRecorderDriven because it was difficult to use (large recordings, execution errors failing the test, etc.) -- issues this PR tries to address. There are some real limitations here though that may still make this approach untenable: When --rewrite is used for TestRecorderDriven, because it shells out, will: - takes time proportional to the actual dev invocations; - makes it difficult to run under bazel (through without --rewrite it works just fine); The few remaining tests for this recorder stuff are probably examples of dev doing too much, when it should instead push the logic down into bazel rules. As we continue doing so, we should re-evaluate whether this harness provides much value. Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds syntax and existence checks to handle bad package
arguments in a more robust way.
Fixes #73457.
Release note: None