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.

Sign up for GitHub

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

Jump to bottom

Improve test output to include correct location #2058

Merged
merged 4 commits into from
Jan 2, 2025
Merged

Improve test output to include correct location #2058

merged 4 commits into from
Jan 2, 2025
+69 −14

Conversation

denik
Copy link
Contributor

@denik denik commented Dec 31, 2024

Changes

  • Add t.Helper() in testcli-related helpers, this ensures that output is attributed correctly to test case and not to the helper.
  • Modify testlcli.Run() to run process in foreground. This is needed for t.Helper to work.
  • Extend a few assertions with message to help attribute it to proper helper where needed.

Tests

Manually reviewed test output.

Before:

+ go test --timeout 3h -v -run TestDefaultPython/3.9 ./integration/bundle/
=== RUN   TestDefaultPython
=== RUN   TestDefaultPython/3.9
    workspace.go:26: aws
    golden.go:14: run args: [bundle, init, default-python, --config-file, config.json]
    runner.go:206: [databricks stderr]:
    runner.go:206: [databricks stderr]: Welcome to the default Python template for Databricks Asset Bundles!
...
    testdiff.go:23:
                Error Trace:    /Users/denis.bilenko/work/cli/libs/testdiff/testdiff.go:23
                                                        /Users/denis.bilenko/work/cli/libs/testdiff/golden.go:43
                                                        /Users/denis.bilenko/work/cli/internal/testcli/golden.go:23
                                                        /Users/denis.bilenko/work/cli/integration/bundle/init_default_python_test.go:92
                                                        /Users/denis.bilenko/work/cli/integration/bundle/init_default_python_test.go:45
...

After:

+ go test --timeout 3h -v -run TestDefaultPython/3.9 ./integration/bundle/
=== RUN   TestDefaultPython
=== RUN   TestDefaultPython/3.9
    init_default_python_test.go:51: CLOUD_ENV=aws
    init_default_python_test.go:92:   args: bundle, init, default-python, --config-file, config.json
    init_default_python_test.go:92: stderr:
    init_default_python_test.go:92: stderr: Welcome to the default Python template for Databricks Asset Bundles!
...
    init_default_python_test.go:92:
                Error Trace:    /Users/denis.bilenko/work/cli/libs/testdiff/testdiff.go:24
                                                        /Users/denis.bilenko/work/cli/libs/testdiff/golden.go:46
                                                        /Users/denis.bilenko/work/cli/internal/testcli/golden.go:23
                                                        /Users/denis.bilenko/work/cli/integration/bundle/init_default_python_test.go:92
                                                        /Users/denis.bilenko/work/cli/integration/bundle/init_default_python_test.go:45
...

@denik
Improve test output to include correct location
dc95332 
- Add t.Helper() in testcli-related helpers, this ensures that output
  is attributed correctly to test case and not to the helper.
- Modify testlcli.Run() to run process in foreground. This is needed for t.Helper to work.
- Extend a few assertions with message to help attribute it to proper helper where needed.
@denik denik temporarily deployed to test-trigger-is December 31, 2024 11:39 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 31, 2024 11:39 — with GitHub Actions Inactive
@denik denik marked this pull request as ready for review December 31, 2024 11:51
shreyas-goenka
shreyas-goenka reviewed Jan 2, 2025
View reviewed changes
internal/testcli/runner.go Outdated Show resolved Hide resolved
internal/testcli/runner.go Outdated
// once test has been executed. This is needed because flag values reside
// in a global singleton data-structure, and thus subsequent tests might
// otherwise interfere with each other
r.registerFlagCleanup(cli)
Copy link
Contributor

Choose a reason for hiding this comment

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

This used to be true once in ancient times but is not true anymore. The command objects are not singleton anymore, cmd.New(ctx) initializes a completely new command tree.

We should be able to remove this bit and the RunBackground function from here, and the function itself should also be fine to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this bit of context! I've cleaned up both SetContext and registerFlagCleanup from this PR.

Will clean up it fully in follow up.

Copy link
Contributor Author

@denik denik Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The integration tests failed after the removal: https://github.com/databricks-eng/eng-dev-ecosystem/actions/runs/12579907431/job/35060907953

UPDATE: seems unrelated.

internal/testcli/golden.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Note a couple of other helpers are present in:

  1. integration/libs/filer/helpers_test.go
  2. integration/libs/filer/filer_test.go (filerTest)
  3. integration/python/python_tasks_test.go (runPythonTasks etc)
  4. integration/enforce_convention_test.go (enumeratePackages)

And probably more...

Feel free to skip them though if you consider them to be out of scope for this PR.

Copy link
Contributor Author

@denik denik Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

True, this PR does attempt to fix all. I am inclined to add them everywhere and add linter to enforce that though (in follow-up PRs).

@denik denik temporarily deployed to test-trigger-is January 2, 2025 08:56 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is January 2, 2025 08:56 — with GitHub Actions Inactive
@denik denik requested a review from shreyas-goenka January 2, 2025 08:59
@denik denik temporarily deployed to test-trigger-is January 2, 2025 09:17 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is January 2, 2025 09:18 — with GitHub Actions Inactive
pietern
pietern reviewed Jan 2, 2025
View reviewed changes
internal/testcli/runner.go
//nolint:staticcheck // cobra sets the context and doesn't clear it
cli.SetContext(nil)

return stdout, stderr, err
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the nuance around running this in the main goroutine vs a background one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t.Helper() mechanism does not work for background goroutine.

@denik denik temporarily deployed to test-trigger-is January 2, 2025 09:35 — with GitHub Actions Inactive
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 2, 2025

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 2058
  • Commit SHA: c033631b93740f4509fb9ec8df61c5c16c1d0f0a

Checks will be approved automatically on success.

@denik denik temporarily deployed to test-trigger-is January 2, 2025 09:35 — with GitHub Actions Inactive
@denik denik merged commit 3f75240 into main Jan 2, 2025
8 of 9 checks passed
@denik denik deleted the denik/foreground-testcli branch January 2, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern left review comments

@shreyas-goenka shreyas-goenka shreyas-goenka approved these changes

@andrewnester andrewnester Awaiting requested review from andrewnester andrewnester is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@denik @pietern @shreyas-goenka