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

Add regression tests for CLI error output #1566

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

kanterov
Copy link
Contributor

@kanterov kanterov commented Jul 3, 2024

Changes

Add regression tests for #1563

We test 2 code paths:

  • if there is an error, we can print to stderr
  • if there is a valid output, we can print to stdout

We should also consider adding black-box tests that will run the CLI binary as a black box and inspect its output to stderr/stdout.

Tests

Unit tests

@kanterov kanterov requested a review from pietern July 4, 2024 08:17
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Could you also modify cobraTestRunner in internal/helpers.go to use the same?

main.go Outdated
Comment on lines 13 to 15
code := root.Execute(ctx, cmd.New(ctx))

os.Exit(code)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
code := root.Execute(ctx, cmd.New(ctx))
os.Exit(code)
code := root.Execute(ctx, cmd.New(ctx))
os.Exit(code)

Comment on lines 1 to 2
package root_test // using 'root' will create circular import
import (
Copy link
Contributor

Choose a reason for hiding this comment

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

The circular import thing is implied. We use the same pattern in other places.

Suggested change
package root_test // using 'root' will create circular import
import (
package root_test
import (

Copy link
Contributor

@lennartkats-db lennartkats-db left a comment

Choose a reason for hiding this comment

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

PTAL at Pieter's comments, and please make the PR title a bit more descriptive for changelog purposes, otherwise LGTM

@kanterov kanterov changed the title Add tests for Execute Add regression tests for CLI error output Jul 4, 2024
@kanterov
Copy link
Contributor Author

kanterov commented Jul 4, 2024

@pietern I didn't know cobraTestRunner existed. I've fixed its fidelity, now it uses root.Execute and logs to stderr. I think we don't need root_test.go anymore, because we can use cobra tests instead.

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

I'm triggering an integration test run to confirm they pass with the modified helper.

@kanterov
Copy link
Contributor Author

kanterov commented Jul 5, 2024

@pietern is this good to merge?

@kanterov kanterov requested a review from pietern July 8, 2024 12:05
@pietern
Copy link
Contributor

pietern commented Jul 10, 2024

Yes, good to go!

@pietern pietern enabled auto-merge July 10, 2024 06:36
@pietern pietern added this pull request to the merge queue Jul 10, 2024
Merged via the queue into databricks:main with commit 25737bb Jul 10, 2024
5 checks passed
andrewnester added a commit that referenced this pull request Jul 10, 2024
Bundles:
 * Override complex variables with target overrides instead of merging ([#1567](#1567)).
 * Rewrite local path for libraries in foreach tasks ([#1569](#1569)).
 * Change SetVariables mutator to mutate dynamic configuration instead ([#1573](#1573)).
 * Return early in bundle destroy if no deployment exists ([#1581](#1581)).
 * Let notebook detection code use underlying metadata if available ([#1574](#1574)).
 * Remove schema override for variable default value ([#1536](#1536)).
 * Print diagnostics in 'bundle deploy' ([#1579](#1579)).

Internal:
 * Update actions/upload-artifact to v4 ([#1559](#1559)).
 * Use Go 1.22 to build and test ([#1562](#1562)).
 * Move bespoke status call to main workspace files filer ([#1570](#1570)).
 * Add new template ([#1578](#1578)).
 * Add regression tests for CLI error output ([#1566](#1566)).

Dependency updates:
 * Bump golang.org/x/mod from 0.18.0 to 0.19.0 ([#1576](#1576)).
 * Bump golang.org/x/term from 0.21.0 to 0.22.0 ([#1577](#1577)).
@andrewnester andrewnester mentioned this pull request Jul 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 10, 2024
Bundles:
* Override complex variables with target overrides instead of merging
([#1567](#1567)).
* Rewrite local path for libraries in foreach tasks
([#1569](#1569)).
* Change SetVariables mutator to mutate dynamic configuration instead
([#1573](#1573)).
* Return early in bundle destroy if no deployment exists
([#1581](#1581)).
* Let notebook detection code use underlying metadata if available
([#1574](#1574)).
* Remove schema override for variable default value
([#1536](#1536)).
* Print diagnostics in 'bundle deploy'
([#1579](#1579)).

Internal:
* Update actions/upload-artifact to v4
([#1559](#1559)).
* Use Go 1.22 to build and test
([#1562](#1562)).
* Move bespoke status call to main workspace files filer
([#1570](#1570)).
* Add new template
([#1578](#1578)).
* Add regression tests for CLI error output
([#1566](#1566)).

Dependency updates:
* Bump golang.org/x/mod from 0.18.0 to 0.19.0
([#1576](#1576)).
* Bump golang.org/x/term from 0.21.0 to 0.22.0
([#1577](#1577)).
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