-
Notifications
You must be signed in to change notification settings - Fork 53
Add integration tests using Testcontainers #354
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
doringeman
left a comment
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! Why isn't the integration-tests rule in the root Makefile?
3491233 to
2c7b497
Compare
2c7b497 to
7fdcd28
Compare
… for insecure access
…digest references
# Conflicts: # cmd/cli/go.mod
| @@ -0,0 +1,37 @@ | |||
| name: Integration Tests | |||
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.
I'm adding this additional workflow because I have to build the DMR image before running tests. This temporary, integration test should be run in the normal CI workflow
| tag string // e.g., "latest" | ||
| registry string // e.g., "registry.local:5000" | ||
| modelID string // Full ID: sha256:... | ||
| digest string // sha256:... |
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.
Note: modelID and digest are exactly the same (the digest of the manifest) I just added 2 fields to make it more readable
I keep it in a separated workflow for now because we need to build DMR image with current version of DMR, otherwise we cannot define the default registry |
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.
Hey there - I've reviewed your changes - here's some feedback:
- integration_test.go is very large—consider breaking it into smaller files or extracting common setup/teardown logic into helper functions to improve readability and maintainability.
- The go.mod diff includes a lot of indirect module updates; please run
go mod tidyto remove unused dependencies and keep the module file clean. - Instead of calling testcontainers.CleanupContainer/Network directly, register cleanup with t.Cleanup so resources are always cleaned up even if setup fails early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- integration_test.go is very large—consider breaking it into smaller files or extracting common setup/teardown logic into helper functions to improve readability and maintainability.
- The go.mod diff includes a lot of indirect module updates; please run `go mod tidy` to remove unused dependencies and keep the module file clean.
- Instead of calling testcontainers.CleanupContainer/Network directly, register cleanup with t.Cleanup so resources are always cleaned up even if setup fails early.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| - name: Run integration tests | ||
| working-directory: cmd/cli/commands | ||
| run: go test -v -tags=integration -timeout=5m -run TestIntegration_PullModel |
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.
Why only PullModel?
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 | ||
|
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@44694675825211faa026b3c33043df3e48a5fa00 | ||
| with: | ||
| go-version: 1.24.2 | ||
| cache: true | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 | ||
|
|
||
| - name: Run integration tests | ||
| run: make integration-tests |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix the problem, explicitly define a permissions block with the least privilege needed. Since this workflow job (integration-test) only checks out code and runs tests but does not perform any write operations, the minimal permission needed is contents: read. The best approach is to add:
permissions:
contents: readThis block can be added at the workflow root (applies to all jobs), or more specifically, to the integration-test job (since there’s only one job here, either location is functionally the same). Place the block above or within the relevant job definition.
To implement this, add permissions: contents: read just below runs-on: ubuntu-latest and above timeout-minutes: 10 (line 9 in the snippet). No other changes or dependency modifications are necessary.
-
Copy modified lines R10-R11
| @@ -7,6 +7,8 @@ | ||
| jobs: | ||
| integration-test: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| timeout-minutes: 10 | ||
|
|
||
| steps: |
…t test runs Signed-off-by: Dorin Geman <[email protected]>
| @@ -0,0 +1,464 @@ | |||
| //go:build 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.
suggestion: I'd avoid using the build tag unless necessary. Why? Because then you'll be running tests twice (adding the flag in the make file will run all AND will include files with this tag), so you can end up adding exclusions and complex boolean logic for this. My recommendation is to run them altogether, and treat in-process integration tests as unit tests, as they are blazing fast thanks to containers.
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.
That's a good point, thank you!
|
Minor nit, logs report error when we have a valid fallback: |
This pull request introduces integration testing support for the CLI, enabling end-to-end validation of model registry and runner interactions using Docker containers.
model-cli (what its being tested) <----> DMR (Testcontainer) <----> OCI Registry (Testcontainer)
The registry is set as the default one in here.
We can easily add more tests, for other commands like
tag,configure, etc, but it can be done in a followup PR.I added a separated workflow because it needs to build the DMR image (the current
latestdoes not include the env variable to customize default registry yet), once we publish a newlatestwe can move the workflow to the normal CI.The PR also contains a fix for the
inspectcommand.@mdelapenya could you please take a look? Let us know any recommendation you might have 🙏