Skip to content

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

Merged
merged 16 commits into from
Jul 15, 2025

Conversation

mwojtyczka
Copy link
Contributor

@mwojtyczka mwojtyczka commented Jul 15, 2025

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.

- name: Run integration tests
  uses: databrickslabs/sandbox/acceptance@acceptance/v0.4.4
  with:
    vault_uri: ${{ secrets.VAULT_URI }}
    timeout: 2h
    codegen_path: tests/integration/.codegen.json

- name: Run e2e tests
  uses: databrickslabs/sandbox/acceptance@acceptance/v0.4.4
  with:
    vault_uri: ${{ secrets.VAULT_URI }}
    timeout: 2h
    codegen_path: tests/e2e/.codegen.json

tests/integration/.codegen.json:

{
  "version": {
    "src/databricks/labs/dqx/__about__.py": "__version__ = \"$VERSION\""
  },
  "toolchain": {
    "required": ["python3", "hatch"],
    "pre_setup": ["hatch env create"],
    "prepend_path": ".venv/bin",
    "acceptance_path": "tests/integration"
  }
}

tests/e2e/.codegen.json:

{
  "version": {
    "src/databricks/labs/dqx/__about__.py": "__version__ = \"$VERSION\""
  },
  "toolchain": {
    "required": ["python3", "hatch"],
    "pre_setup": ["hatch env create"],
    "prepend_path": ".venv/bin",
    "acceptance_path": "tests/e2e"
  }
}

The change is backward compatible so the old behaviour is retained:

- name: Run integration tests
  uses: databrickslabs/sandbox/acceptance@acceptance/v0.4.4
  with:
    vault_uri: ${{ secrets.VAULT_URI }}
    timeout: 2h

This will search for .codegen.json in the code and use the first codegen found.

Copy link

github-actions bot commented Jul 15, 2025

✅ 5/5 passed, 1 skipped, 20s total

Running from acceptance #253

Copilot

This comment was marked as outdated.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

@mwojtyczka mwojtyczka requested a review from a team as a code owner July 15, 2025 11:17
@mwojtyczka mwojtyczka requested a review from Copilot July 15, 2025 11:19
Copilot

This comment was marked as outdated.

Copy link
Contributor

@Copilot Copilot AI left a 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) from getFileContent instead of hard-coding fs.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 in FromFileset; please add tests for cases when codegenPath is provided, empty, and missing to ensure correct behavior.
func FromFileset(files fileset.FileSet, codegenPath *string) (*Toolchain, error) {

Copy link
Collaborator

@sundarshankar89 sundarshankar89 left a 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"
Copy link
Collaborator

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

Copy link

@asnare asnare left a 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.)

@sundarshankar89 sundarshankar89 merged commit 80d136e into main Jul 15, 2025
7 checks passed
@sundarshankar89 sundarshankar89 deleted the codegen branch July 15, 2025 13:00
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