Skip to content

test: cover Python and Go source normalization - #22

Open
giri256 wants to merge 1 commit into
AgentsKit-io:mainfrom
giri256:issue-21-source-fixtures
Open

giri256 wants to merge 1 commit into
AgentsKit-io:mainfrom
giri256:issue-21-source-fixtures

Conversation

@giri256

@giri256 giri256 commented Aug 3, 2026

Copy link
Copy Markdown

What changed

Adds credential-free source-normalization coverage for Python and Go fixtures. The test also confirms Markdown files are ignored, path inputs are unchanged, and Python stdin input is marked as changed.

Why

Closes #21. The repository supports several source languages, but the checked-in fixtures only exercised TypeScript behavior.

Verification

  • npm run check
  • npm pack --dry-run
  • node --test test/sources.test.mjs
  • No public CLI or Action behavior changed
  • No secrets, generated snapshots, dependencies, or model output included

@EmersonBraun

Copy link
Copy Markdown
Member

Code review

No issues found. The contribution matches issue #21: it adds the requested Python and Go fixtures, keeps Markdown out of targets, verifies complete contents and language detection, covers path versus stdin isChanged, and avoids production-code or dependency changes.

@EmersonBraun EmersonBraun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@EmersonBraun

Copy link
Copy Markdown
Member

Thanks for this — the Python/Go coverage is a real gap this closes. Before merging, though: I applied this patch on top of current `main` to check it still passes (this branch is a few weeks behind), and one assertion now fails:

```
local source directories normalize supported files and ignore Markdown
AssertionError: expected ['example.go', 'example.py']
actual: ['README.md', 'example.go', 'example.py']
```

This isn't staleness in the usual "just rebase" sense — `.md` was added to the supported extensions list in `agents/code-review/sources.ts` since this PR was opened (Markdown is now a reviewable target, to support product-doc review). So `test/fixtures/sources/README.md` is genuinely picked up now, and the "ignore Markdown" premise in the test needs to change.

Two ways to fix it, your call:

  • Update the assertion to expect all three files (`README.md`, `example.go`, `example.py`) and rename the test/comment away from "ignore Markdown", since that's no longer the behavior being covered — or
  • If you'd still like an explicitly-ignored fixture in this test, rename `README.md` to something outside the supported-extension list (e.g. a `.txt` note) so the "ignored file" assertion stays meaningful.

Could you push either fix and rebase onto `main`? Happy to re-review right away once it's up — this is otherwise ready to merge (the Go/Python normalization coverage itself is correct and needed).

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.

test: add multi-language source-normalization fixtures

2 participants