Skip to content

feat(topograph): Integration Test Support#205

Merged
ravisoundar merged 7 commits into
mainfrom
rs-func-test
Feb 6, 2026
Merged

feat(topograph): Integration Test Support#205
ravisoundar merged 7 commits into
mainfrom
rs-func-test

Conversation

@ravisoundar
Copy link
Copy Markdown
Collaborator

No description provided.

@ravisoundar ravisoundar requested a review from dmitsh as a code owner February 4, 2026 01:17
@ravisoundar ravisoundar marked this pull request as draft February 4, 2026 01:19
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

  • Adds integration-test oriented behavior to the test provider: parameterized HTTP response codes and loading embedded model fixtures by modelFileName.
  • Updates server generate handler to optionally short-circuit and immediately return simulated error responses for provider=test based on request params.
  • Refactors model parsing to support NewModelFromData, enabling embedded model loading.
  • Updates various tests and JSON payload fixtures to use modelFileName instead of the deprecated model_path parameter; adds a new integration test file under pkg/server/.

Confidence Score: 2/5

  • This PR has a couple of issues that should be fixed before merge (notably, a non-test package importing tests/ and a new test file that won’t compile as-is).
  • The core changes are straightforward, but pkg/providers/test now depends on github.com/NVIDIA/topograph/tests, and pkg/server/integration_test.go references an undefined helper, which would block CI compilation/testing.
  • pkg/providers/test/test.go, pkg/server/integration_test.go, pkg/server/http_server_test.go

Important Files Changed

Filename Overview
.gitignore Adds .vscode to gitignore; no functional impact.
README.md Documents new test-provider parameters (generateResponseCode/topologyResponseCode/modelFileName/errorMessage) and removes model_path example; doc change only.
pkg/models/model.go Refactors model loading to add NewModelFromData and have NewModelFromFile delegate; small, safe change.
pkg/providers/test/test.go Adds request short-circuiting and embedded model loading via tests package; introduces a prod dependency on github.com/NVIDIA/topograph/tests.
pkg/server/engine_test.go Updates engine tests to use modelFileName and new embedded-model error message expectations.
pkg/server/http_server.go Adds generate-handler short-circuit for test provider responses based on provider params.
pkg/server/http_server_test.go Adds modelFileName to payload constants but leaves deprecated model_path fields present, making tests inconsistent with provider params.
pkg/server/integration_test.go Adds new server integration test harness; currently won’t compile due to missing stringToLineMap helper and has retry semantics already flagged in earlier threads.
tests/model.go Adds embedded models FS helper under tests package; OK for tests but problematic when imported by non-test code.
tests/payloads/test-toposim-block-fake.json Updates test payload to use modelFileName instead of model_path.
tests/payloads/test-toposim-block.json Updates test payload to use modelFileName instead of model_path.
tests/payloads/test-toposim-tree.json Updates test payload to use modelFileName instead of model_path.

Sequence Diagram

sequenceDiagram
    participant C as Client
    participant HS as HttpServer (/v1/generate)
    participant TP as Test provider (pkg/providers/test)
    participant Q as Async queue
    participant PR as processRequest
    participant RES as Result store (/v1/topology)

    C->>HS: POST /v1/generate (topology.Request JSON)
    HS->>HS: readRequest() + validate()
    alt provider == "test" and generateResponseCode != 202
        HS->>TP: HandleTestProviderRequest(tr)
        TP-->>HS: http.Error(..., generateResponseCode)
        HS-->>C: error status + message
    else provider == "test" and generateResponseCode == 202
        HS->>TP: HandleTestProviderRequest(tr)
        TP-->>HS: false (continue normal flow)
        HS->>Q: Submit(tr)
        Q-->>HS: uid
        HS-->>C: 202 Accepted + uid
    else provider != "test"
        HS->>Q: Submit(tr)
        Q-->>HS: uid
        HS-->>C: 202 Accepted + uid
    end

    C->>RES: GET /v1/topology?uid=...
    RES->>Q: Get(uid)
    Q-->>RES: status + payload/message
    alt status == 200
        RES-->>C: 200 + topology config
    else status == 202
        RES-->>C: 202 + in-progress message
    else error
        RES-->>C: error code + message
    end
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment thread README.md Outdated
Comment thread pkg/providers/test/test.go Outdated
Comment thread pkg/providers/test/test.go Outdated
Comment thread pkg/providers/test/test.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 4, 2026

Codecov Report

❌ Patch coverage is 78.04878% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.88%. Comparing base (0b06b65) to head (8e79a75).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/providers/test/test.go 73.52% 5 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
+ Coverage   65.60%   65.88%   +0.28%     
==========================================
  Files          81       82       +1     
  Lines        4448     4482      +34     
==========================================
+ Hits         2918     2953      +35     
+ Misses       1418     1416       -2     
- Partials      112      113       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ravisoundar ravisoundar marked this pull request as ready for review February 5, 2026 05:23
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment thread pkg/server/integration_test.go Outdated
Comment thread pkg/server/integration_test.go Outdated
Comment thread pkg/server/integration_test.go Outdated
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Comment thread pkg/providers/test/test.go Outdated
Comment thread pkg/providers/test/test.go
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment thread pkg/server/integration_test.go Outdated
Comment thread pkg/server/integration_test.go
Comment thread pkg/server/http_server_test.go
Comment thread pkg/server/http_server_test.go
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

12 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 6, 2026

Additional Comments (2)

pkg/server/http_server.go
Test provider bypasses queue

generate() now short-circuits when provider.name == "test" and returns immediately, so /v1/generate no longer enqueues the request and no uid is returned. But pkg/server/integration_test.go (and existing async flow) assumes /v1/generate returns a uid to poll /v1/topology. As written, any integration payload expecting 202 will get an empty body and then poll uid=="", yielding a 400 instead of exercising the async path. Consider moving this simulation behavior into the provider/queue processing (or still returning a uid while storing the simulated result in srv.async.queue) so the integration test can actually validate the end-to-end flow.


pkg/server/http_server_test.go
Test still uses model_path

These test payload strings still include the deprecated model_path parameter alongside modelFileName. The test provider params struct no longer decodes model_path, so this field is dead and contradicts the earlier change to remove it. This should be removed from the test payloads so tests reflect the supported API surface.

    "params": {
	  "modelFileName": "medium.yaml"
    }

dmitsh
dmitsh previously approved these changes Feb 6, 2026
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment thread pkg/server/integration_test.go
Comment thread pkg/server/integration_test.go
Comment thread pkg/server/http_server_test.go
Comment thread pkg/server/http_server_test.go
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

12 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment thread pkg/providers/test/test.go
Comment thread pkg/server/http_server_test.go
Comment thread pkg/server/integration_test.go
@ravisoundar ravisoundar requested a review from dmitsh February 6, 2026 23:41
@ravisoundar ravisoundar merged commit f392302 into main Feb 6, 2026
6 checks passed
@ravisoundar ravisoundar deleted the rs-func-test branch February 6, 2026 23:44
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.

2 participants