-
Notifications
You must be signed in to change notification settings - Fork 22
Added optional path to the codegen file for configuring the acceptance tests #502
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
Conversation
✅ 5/5 passed, 1 skipped, 20s total Running from acceptance #253 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds an optional codegen_path
parameter to the acceptance tests GitHub Action, enabling separate configuration of different test suites via custom .codegen.json
files.
- Extended
FromFileset
to accept an optional path to a codegen file. - Updated all callers and the action metadata to propagate
codegen_path
. - Enhanced documentation and examples to demonstrate the new parameter.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
go-libs/toolchain/toolchain.go | Updated FromFileset signature to accept codegenPath and refactored file filtering logic. |
go-libs/linkdev/repo.go | Updated FromFileset call to pass nil for the new optional parameter. |
acceptance/main.go | Read codegen_path input and injected it into the context for downstream use. |
acceptance/ecosystem/python.go | Retrieved codegen_path from context and forwarded it to FromFileset . |
acceptance/action.yml | Added the codegen_path input parameter to the action definition. |
acceptance/README.md | Updated usage instructions and workflow examples to include the codegen_path option. |
Comments suppressed due to low confidence (3)
go-libs/toolchain/toolchain.go:16
- Removing the exported
ErrNotExist
variable is a breaking change to the public API; consider preserving it (or providing a replacement) to maintain backward compatibility for existing callers.
)
go-libs/toolchain/toolchain.go:35
- [nitpick] Consider wrapping and returning the original error (
err
) fromgetFileContent
instead of hard-codingfs.ErrNotExist
, so the message includes the actual requested path and underlying cause for better debugging.
return nil, fmt.Errorf("provided 'codegen_path' does not exist in the project: %w", fs.ErrNotExist)
go-libs/toolchain/toolchain.go:18
- No unit tests have been added for the new
codegenPath
logic inFromFileset
; please add tests for cases whencodegenPath
is provided, empty, and missing to ensure correct behavior.
func FromFileset(files fileset.FileSet, codegenPath *string) (*Toolchain, error) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
"required": ["python3", "hatch"], | ||
"pre_setup": ["hatch env create"], | ||
"prepend_path": ".venv/bin", | ||
"acceptance_path": "tests/integration" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding Readme.md
I like this change. Today we handle this with this plumbing approach, which simplifies things drastically https://github.com/databrickslabs/lakebridge/blob/main/tests/integration/conftest.py#L31
Tagging @asnare for any impact on UCX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(Tested with UCX on databrickslabs/ucx#4224 to be sure.)
Improved github action for acceptance tests to accept optionally path to the
.codegen.json
for configuring the tests. This makes it possible to run multiple type of tests separately within one code based.Details:
The github action for acceptance tests
databrickslabs/sandbox/acceptance@acceptance/v0.4.3
uses the first.codegen.json
file found in the project. For that reason it is currently not possible to run different type of tests separately using this action.With the proposed changes, a path to the codegen can be optionally provided, e.g.
tests/integration/.codegen.json:
tests/e2e/.codegen.json:
The change is backward compatible so the old behaviour is retained:
This will search for
.codegen.json
in the code and use the first codegen found.