Fix streaming of stdout, stdin, stderr in cobra test runner #1742
+17
−2
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
|
Triggered the nightlies on this... |
shreyas-goenka
requested review from
andrewnester,
lennartkats-db and
pietern
and removed request for
lennartkats-db
pietern
reviewed
Sep 2, 2024
shreyas-goenka
commented
Sep 2, 2024
| @@ -29,6 +29,12 @@ func (f *progressLoggerFlag) resolveModeDefault(format flags.ProgressLogFormat) | |||
| } | |||
|
|
|||
| func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Context, error) { | |||
| // No need to initialize the logger if it's already set in the context. This | |||
| // happens in unit tests where the logger is setup as a fixture. | |||
| if _, ok := cmdio.FromContext(ctx); ok { | |||
There was a problem hiding this comment.
Before, we would not use the stdin/out/err io readers/writers in internal.NewCobraTestRunner(). This passes them through now.
| _, _, err = c.Run() | ||
| require.ErrorContains(t, err, "failed to bind the resource") | ||
| // Bind should fail because prompting is not possible. | ||
| stdout, stderr, err := blackBoxRun(t, bundleRoot, "bundle", "deployment", "bind", "foo", fmt.Sprint(jobId)) |
There was a problem hiding this comment.
The original test fails because it implicitly relied on the stdin not being passed to the cobra test runner, which caused an error here because of EOF on stdin.
The test is now improved, and the error message is more prescriptive to use the --auto-approve flag. Testing the prompting library itself is out of scope for this test.
|
Nightlies are green and happy with this PR. |
pietern
approved these changes
Sep 2, 2024
andrewnester
added a commit
that referenced
this pull request
Sep 5, 2024
CLI: * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)). Bundles: * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)). * Add prompt when a pipeline recreation happens ([#1672](#1672)). * Use materialized views in the default-sql template ([#1709](#1709)). * Update templates to latest LTS DBR ([#1715](#1715)). * Make lock optional in the JSON schema ([#1738](#1738)). * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)). * Include a permissions section in all templates ([#1713](#1713)). * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)). * Fixed variable override in target with full variable syntax ([#1749](#1749)). Internal: * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)). * PythonMutator: explain missing package error ([#1736](#1736)). * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)). * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)). Dependency updates: * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)).
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 5, 2024
CLI: * Do not error if we cannot prompt for a profile in `auth login` ([#1745](#1745)). Bundles: As of this release CLI will show a prompt is if there are configuration changes which will lead to a DLT recreation. Users can skip the prompt by specifying the `--auto-approve` flag * Pass along $AZURE_CONFIG_FILE to Terraform process ([#1734](#1734)). * Add prompt when a pipeline recreation happens ([#1672](#1672)). * Use materialized views in the default-sql template ([#1709](#1709)). * Update templates to latest LTS DBR ([#1715](#1715)). * Make lock optional in the JSON schema ([#1738](#1738)). * Do not suppress normalisation diagnostics for resolving variables ([#1740](#1740)). * Include a permissions section in all templates ([#1713](#1713)). * Fixed complex variables are not being correctly merged from include files ([#1746](#1746)). * Fixed variable override in target with full variable syntax ([#1749](#1749)). Internal: * Consider serverless clusters as compatible for Python wheel tasks ([#1733](#1733)). * PythonMutator: explain missing package error ([#1736](#1736)). * Add `dyn.Time` to box a timestamp with its original string value ([#1732](#1732)). * Fix streaming of stdout, stdin, stderr in cobra test runner ([#1742](#1742)). Dependency updates: * Bump github.com/Masterminds/semver/v3 from 3.2.1 to 3.3.0 ([#1741](#1741)). --------- Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
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.
Changes
We were not using the readers and writers set in the test fixtures in the progress logger. This PR fixes that. It also modifies
TestAccAbortBind, which was implicitly relying on the bug.I encountered this bug while working on #1672.
Tests
Manually.
From non-tty:
From tty, bind works as expected.