Feature/review of integration test#307
Merged
Merged
Conversation
#305) - Added integration test specification in SPEC.md to outline the scope and structure of tests. - Created conftest.py for shared fixtures and helpers for offline integration tests. - Implemented test_full_pipeline_snapshot.py to validate the complete release notes generation pipeline. - Added test_bulk_sub_issue_collector.py for live testing of BulkSubIssueCollector with real GitHub API. - Created fixtures/test_full_pipeline_snapshot.md to store expected output for snapshot tests. - Added initial structure for live tests in the live directory.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughSeparates offline and live integration test jobs in CI, excludes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
miroslavpojer
added
the
work in progress
Contributor
There was a problem hiding this comment.
Pull request overview
Creates a revamped integration-testing approach: a comprehensive offline integration suite that exercises main.run() end-to-end using mocked GitHub/DataMiner, plus a small live smoke test for BulkSubIssueCollector.
Changes:
- Replaces the legacy integration snapshot test with a large offline pipeline test suite + a golden snapshot fixture.
- Splits integration testing into offline vs live, updating docs and CI workflow accordingly.
- Simplifies
DataMinerto constructBulkSubIssueCollectorwith default config (removing explicitverify_tls=False).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/test_release_notes_snapshot.py | Removes the previous snapshot-style integration test module. |
| tests/integration/test_builder_pipeline.py | Adds extensive offline integration tests that run main.run() and validate output (including snapshot). |
| tests/integration/conftest.py | Adds shared fixtures/factories and environment setup for offline integration tests. |
| tests/integration/fixtures/test_full_pipeline_snapshot.md | Adds the golden snapshot used by the full pipeline snapshot test. |
| tests/integration/live/test_bulk_sub_issue_collector.py | Updates the live smoke test to use default collector config and a repo-local parent issue. |
| tests/integration/live/init.py | Introduces the live integration test package. |
| release_notes_generator/data/miner.py | Removes CollectorConfig usage and instantiates BulkSubIssueCollector with defaults. |
| DEVELOPER.md | Updates integration testing documentation to reflect offline/live split and snapshot regeneration. |
| .github/workflows/test.yml | Updates CI jobs: offline integration ignores tests/integration/live, and live job runs live pytest smoke tests. |
Comments suppressed due to low confidence (1)
tests/integration/live/test_bulk_sub_issue_collector.py:18
- This module has no module docstring, which will trigger Pylint
missing-module-docstring (C0114)under the repo’s current CI linting (it runs Pylint ontests/too). Add a short module docstring at the top (below the license header) to avoid lowering the Pylint score.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
tests/integration/conftest.py (4)
83-92:patch_envreturn-type annotation disallowsNone, but the inner_applyaccepts it.
_applyhas signature(overrides: dict[str, str] | None = None) -> None, yet the fixture advertisesCallable[[dict[str, str]], None]. Every caller that relies on the default (e.g.,capture_run(patch_env)→patch_env(overrides)withoverrides=None) is technically a type error under strict mypy. Tighten the annotation to reflect the actual contract.Proposed fix
-def patch_env(monkeypatch: pytest.MonkeyPatch) -> Callable[[dict[str, str]], None]: +def patch_env(monkeypatch: pytest.MonkeyPatch) -> Callable[[dict[str, str] | None], None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 83 - 92, The fixture patch_env currently declares its return type as Callable[[dict[str, str]], None] but its inner function _apply accepts overrides: dict[str, str] | None = None; update the return-type annotation of patch_env to accept an optional dict (e.g., Callable[[dict[str, str] | None], None] or using Optional) so the signature of patch_env matches _apply exactly; locate the patch_env function and change its annotation accordingly to eliminate the mypy mismatch.
252-278: Usemonkeypatchand robust parsing incapture_run.Two concerns:
os.environ["GITHUB_OUTPUT"] = tmp_pathdirectly mutates process state; if the caller's pytestmonkeypatchhad previously setGITHUB_OUTPUT, theos.environ.pop(...)infinallywill erase that original value instead of restoring it. Prefer takingmonkeypatchas a parameter (or apytest.MonkeyPatchfixture inside a wrapper fixture) and callingmonkeypatch.setenv("GITHUB_OUTPUT", tmp_path), which is automatically undone by pytest.- The payload parser only recognizes the literal heredoc delimiter
EOF. GitHub Actions guidance is to pick a random delimiter to avoid collisions with content; whileset_action_outputin this repo currently emitsEOF, the parser will silently produce a truncated string the moment a release-notes body contains a line equal toEOF. Consider parsing until the matching delimiter read from the header (name<<DELIM) rather than hard-coding"EOF".Proposed fix (parser robustness)
- # Parse the GitHub Actions output format: "name<<EOF\nvalue\nEOF\n" + # Parse the GitHub Actions output format: "name<<DELIM\nvalue\nDELIM\n" lines = raw.splitlines() notes_lines: list[str] = [] - inside = False + delimiter: str | None = None for line in lines: - if line == "release-notes<<EOF": - inside = True + if delimiter is None and line.startswith("release-notes<<"): + delimiter = line.split("<<", 1)[1] continue - if line == "EOF" and inside: + if delimiter is not None and line == delimiter: break - if inside: + if delimiter is not None: notes_lines.append(line) return "\n".join(notes_lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 252 - 278, capture_run currently mutates os.environ directly and hardcodes the heredoc delimiter; change its signature to accept a pytest.MonkeyPatch (e.g., monkeypatch: pytest.MonkeyPatch) or a generic monkeypatch parameter and use monkeypatch.setenv("GITHUB_OUTPUT", tmp_path) instead of os.environ[...] and manual pop/unlink to avoid stomping preexisting env; keep tempfile usage and Path(tmp_path).unlink but remove os.environ manipulation. For parsing, read raw, split lines and detect the header line starting with "release-notes<<"; extract the delimiter token after the "<<" (e.g., delim = header.split("<<",1)[1]) and then collect subsequent lines until you encounter a line equal to that delim (rather than hard-coding "EOF"); keep notes_lines and return "\n".join(notes_lines). Ensure references: capture_run, patch_env, main.run, GITHUB_OUTPUT, notes_lines, inside.
18-36: Import ordering violates PEP 8 / isort grouping.
import mainis a first-party module but is placed in the stdlib group beforeos,tempfile,time, etc. Additionally,mainandpytest_mockare local/third-party imports interleaved with stdlib ones. The groups should be stdlib → third-party → first-party, each separated by a blank line. Pylint (wrong-import-order) will likely flag this oncetests/is linted.Proposed re-ordering
-import main -import os +import os import tempfile import time from collections.abc import Callable from datetime import datetime from pathlib import Path import pytest from pytest_mock import MockerFixture from github.Commit import Commit from github.GitRelease import GitRelease from github.Issue import Issue from github.PullRequest import PullRequest from github.Repository import Repository +import main from release_notes_generator.action_inputs import ActionInputs from release_notes_generator.model.mined_data import MinedData🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 18 - 36, Imports are mis-grouped: move stdlib imports (os, tempfile, time, datetime, pathlib.Path, collections.abc.Callable) first, then a blank line, then third-party imports (pytest, pytest_mock, github.Commit/GitRelease/Issue/PullRequest/Repository), then a blank line, then first-party/local imports (main, release_notes_generator.action_inputs.ActionInputs, release_notes_generator.model.mined_data.MinedData); reorder the import block accordingly (or run isort) so functions/classes like ActionInputs and MinedData and modules main are in the first-party group and pylint/isort import-order warnings are resolved.
44-54: Drop the unnecessarytype: ignore[return].The fixture is declared
-> Noneand implicitly returnsNone; there's no return-related mypy error to suppress. As per coding guidelines ("Avoid type: ignore unless required; document why when used"), this suppression should be removed (or, if retained, annotated with a# mypy: ...explanation).As per coding guidelines: "Avoid type: ignore unless required; document why when used".Proposed fix
-def reset_action_inputs_cache(monkeypatch: pytest.MonkeyPatch) -> None: # type: ignore[return] +def reset_action_inputs_cache(monkeypatch: pytest.MonkeyPatch) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 44 - 54, The fixture reset_action_inputs_cache currently has an unnecessary type ignore on its signature; remove the " # type: ignore[return]" from the def reset_action_inputs_cache(...) annotation so the function is simply declared "def reset_action_inputs_cache(monkeypatch: pytest.MonkeyPatch) -> None:" and leave the body unchanged, ensuring no other mypy suppressions are added; reference the fixture name (reset_action_inputs_cache) and the class ActionInputs in your change..github/workflows/test.yml (1)
171-203: Minor inconsistencies in the live-tests job definition.
- Lines 182 and 187: the
actions/checkout/actions/setup-pythonuses:lines drop the trailing# v4/# v5SHA comment used in every other job. Keep the comment for consistency and to make the pinned version obvious to reviewers.- Line 171: the job key is still
integration-test-real-apiwhile the displaynameis nowLive Integration Tests. Consider renaming the key (e.g.,live-integration-tests) so required-check names in branch protection rules don't diverge from the displayed name.Proposed fix
- integration-test-real-api: + live-integration-tests: name: Live Integration Tests runs-on: ubuntu-latest @@ - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v4 with: fetch-depth: 0 persist-credentials: false - - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 + - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v5Note: renaming the job key is a breaking change for any existing required-check configuration — if such rules exist, update them in lockstep (or leave the key as-is).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 171 - 203, The workflow job uses inconsistent `uses:` comments and a mismatched job key: update the `uses:` lines for the actions/checkout and actions/setup-python steps in the job `integration-test-real-api` to include the same trailing SHA comment (e.g., `# v4` / `# v5`) style used elsewhere for consistency and reviewer clarity, and optionally rename the job key `integration-test-real-api` to `live-integration-tests` (or another name matching the display `name`) if you want branch-protection required-check names to match — if you rename the job key, ensure any branch protection rules referencing the old key are updated in lockstep.tests/integration/live/test_bulk_sub_issue_collector.py (1)
25-36: Smoke test assertions are very weak.The test pings the real GitHub API but the only post-conditions verified are
hasattr(collector, "parents_sub_issues")and that it's adict. Those are structural invariants that hold even ifscan_sub_issues_for_parentssilently no-ops or swallows network errors. Consider asserting at least that (a) the requested parent key ends up incollector.parents_sub_issuesafterscan_sub_issues_for_parents, and (b) the returnednew_parentsvalue from the first iteration is a list (even if empty). Otherwise this test offers little protection against live-API regressions and will not catch the TLS verification change at the call site.Suggested strengthening
while new_parents and iterations < 2: new_parents = collector.scan_sub_issues_for_parents(new_parents) iterations += 1 - assert hasattr(collector, "parents_sub_issues") - assert isinstance(collector.parents_sub_issues, dict) + assert isinstance(collector.parents_sub_issues, dict) + # After at least one scan, the seed parent must be registered in the map. + assert "AbsaOSS/generate-release-notes#1" in collector.parents_sub_issues🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/live/test_bulk_sub_issue_collector.py` around lines 25 - 36, The smoke test test_bulk_sub_issue_collector_smoke is too weak; update it to assert that after calling BulkSubIssueCollector.scan_sub_issues_for_parents the initially requested parent "AbsaOSS/generate-release-notes#1" appears as a key in collector.parents_sub_issues and that the value returned by the first call to scan_sub_issues_for_parents is a list (even if empty). Concretely, call collector.scan_sub_issues_for_parents once, capture its return into first_new_parents, assert isinstance(first_new_parents, list), then assert "AbsaOSS/generate-release-notes#1" in collector.parents_sub_issues (or in collector.parents_sub_issues.keys()) before doing the second iteration or further assertions. Ensure you reference BulkSubIssueCollector, scan_sub_issues_for_parents, and parents_sub_issues in the updated assertions.tests/integration/test_snapshot.py (1)
33-33: Avoid importing fromconftest.py.
conftest.pymodules are a pytest discovery mechanism; they're not guaranteed to be importable by name, and direct imports of their contents are considered an anti-pattern. This also silently depends ontests/andtests/integration/being packages (requires__init__.pyto be present on the import path). Movebuild_mined_dataandcapture_runto a regular helper module (e.g.,tests/integration/_helpers.py) and re-export them inconftest.pyif you still want them auto-loaded as fixtures/helpers.Proposed direction
-from tests.integration.conftest import build_mined_data, capture_run +from tests.integration._helpers import build_mined_data, capture_run…and move the two functions out of
conftest.pyintotests/integration/_helpers.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_snapshot.py` at line 33, The test imports build_mined_data and capture_run directly from conftest.py which is an anti-pattern; move those functions out of conftest into a regular helper module (e.g., an integration helpers module) and update tests to import build_mined_data and capture_run from that helper module instead of conftest.py; optionally re-export the helpers from conftest.py if you still want them auto-discovered as fixtures/helpers so existing fixtures continue to work.tests/integration/test_chapters.py (1)
294-299: Fragile substring assertion.
"Direct commit" not in actualis a substring of the chapter title"Direct commits", so the two assertions on Lines 294–295 collapse into essentially one check. Consider using a unique commit message (e.g."Stray commit content") that cannot overlap with the chapter title, to independently verify that both the heading and the record content are suppressed.♻️ Proposed adjustment
- c1 = make_commit("aaa1234567", "Direct commit") + c1 = make_commit("aaa1234567", "Stray commit content") @@ - assert "### Direct commits" not in actual, "Hidden service chapter must not appear" - assert "Direct commit" not in actual, "Content of hidden service chapter must not appear" + assert "### Direct commits" not in actual, "Hidden service chapter heading must not appear" + assert "Stray commit content" not in actual, "Hidden service chapter record content must not appear"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_chapters.py` around lines 294 - 299, The two assertions collapse because "Direct commit" is a substring of the heading "Direct commits"; change the test to assert absence of the heading "### Direct commits" and separately assert absence of a unique commit message that cannot overlap with that heading (e.g., replace the commit message used in the test data with something like "Stray commit content" and assert that string is not in actual), updating the assertions in tests/integration/test_chapters.py to check both the heading and the unique record content independently (refer to the variable actual and the existing heading string "### Direct commits" and update the second assertion to the new unique message).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Line 51: The pylint invocation currently uses --ignore=tests which only
matches basenames and therefore doesn't exclude files under the tests/
directory; update the command used to compute pylint_score (the line invoking
`pylint --ignore=tests $(git ls-files '*.py') | ...`) to use
--ignore-paths='^tests/.*' instead so paths under tests/ are excluded by regex
(keep the rest of the pipeline—grep/awk/cut—unchanged).
In `@DEVELOPER.md`:
- Around line 150-163: Update the docs to point to the real helper and test:
change the `_capture_run()` reference to `capture_run` and note it lives in
tests/integration/conftest.py, and update the regenerate-snapshot command to
target tests/integration/test_snapshot.py::test_full_pipeline_snapshot instead
of tests/integration/test_builder_pipeline.py::test_full_pipeline_snapshot so
pytest can collect the test correctly.
In `@tests/integration/test_filtering_and_routing.py`:
- Around line 334-391: Test currently bypasses published-at selection by fully
mocking DataMiner.mine_data and passing a precomputed MinedData with since set;
instead patch so the miner's published-vs-created logic runs: replace the full
mock on DataMiner.mine_data with a wrapper that calls the real implementation
(use mocker.patch.object(DataMiner, "mine_data", wraps=DataMiner.mine_data) or
use side_effect to call the original), or remove the explicit since in
build_mined_data (pass since=None) and let DataMiner.compute since via
get_published_at()/get_release().published_at; keep mocks for
check_repository_exists and get_issues_for_pr but ensure DataMiner.mine_data
executes its published-at branch (references: DataMiner.mine_data,
get_published_at, MinedData, build_mined_data).
- Around line 43-83: The test test_skip_labels_exclude_record_from_all_chapters
should explicitly pass the skip-label config to capture_run instead of relying
on the fixture default; update the call to capture_run(...) to include an env
override like {"INPUT_SKIP_RELEASE_NOTES_LABELS": "skip-release-notes"} so the
skip label is clear and self-contained (locate the capture_run invocation near
the end of the test and add the env dict argument).
In `@tests/integration/test_snapshot.py`:
- Around line 42-54: The current _assert_snapshot function auto-writes missing
fixtures because it treats "not path.exists()" the same as WRITE_SNAPSHOTS=1;
change it so only WRITE_SNAPSHOTS=1 triggers writing and skipping: when
os.getenv("WRITE_SNAPSHOTS") == "1" write the snapshot (using FIXTURES_DIR and
_snapshot_path) and call pytest.skip as before, but if the file does not exist
and WRITE_SNAPSHOTS is not set, fail explicitly (e.g., pytest.fail or an
AssertionError) with a message instructing to run with WRITE_SNAPSHOTS=1 to
create the fixture; keep the existing behavior for comparing expected vs actual
when the file exists.
- Around line 57-70: The block comment describing the "Maximal integration
snapshot" should be removed from module scope and placed into the test's
docstring: open the test_full_pipeline_snapshot function and prepend or replace
its existing docstring with the multi-line narrative (or alternatively add the
text to SPEC.md), ensuring the test module contains no narrative comments
outside test functions; remove the original block comment and keep only allowed
section-header comments at module scope.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 171-203: The workflow job uses inconsistent `uses:` comments and a
mismatched job key: update the `uses:` lines for the actions/checkout and
actions/setup-python steps in the job `integration-test-real-api` to include the
same trailing SHA comment (e.g., `# v4` / `# v5`) style used elsewhere for
consistency and reviewer clarity, and optionally rename the job key
`integration-test-real-api` to `live-integration-tests` (or another name
matching the display `name`) if you want branch-protection required-check names
to match — if you rename the job key, ensure any branch protection rules
referencing the old key are updated in lockstep.
In `@tests/integration/conftest.py`:
- Around line 83-92: The fixture patch_env currently declares its return type as
Callable[[dict[str, str]], None] but its inner function _apply accepts
overrides: dict[str, str] | None = None; update the return-type annotation of
patch_env to accept an optional dict (e.g., Callable[[dict[str, str] | None],
None] or using Optional) so the signature of patch_env matches _apply exactly;
locate the patch_env function and change its annotation accordingly to eliminate
the mypy mismatch.
- Around line 252-278: capture_run currently mutates os.environ directly and
hardcodes the heredoc delimiter; change its signature to accept a
pytest.MonkeyPatch (e.g., monkeypatch: pytest.MonkeyPatch) or a generic
monkeypatch parameter and use monkeypatch.setenv("GITHUB_OUTPUT", tmp_path)
instead of os.environ[...] and manual pop/unlink to avoid stomping preexisting
env; keep tempfile usage and Path(tmp_path).unlink but remove os.environ
manipulation. For parsing, read raw, split lines and detect the header line
starting with "release-notes<<"; extract the delimiter token after the "<<"
(e.g., delim = header.split("<<",1)[1]) and then collect subsequent lines until
you encounter a line equal to that delim (rather than hard-coding "EOF"); keep
notes_lines and return "\n".join(notes_lines). Ensure references: capture_run,
patch_env, main.run, GITHUB_OUTPUT, notes_lines, inside.
- Around line 18-36: Imports are mis-grouped: move stdlib imports (os, tempfile,
time, datetime, pathlib.Path, collections.abc.Callable) first, then a blank
line, then third-party imports (pytest, pytest_mock,
github.Commit/GitRelease/Issue/PullRequest/Repository), then a blank line, then
first-party/local imports (main,
release_notes_generator.action_inputs.ActionInputs,
release_notes_generator.model.mined_data.MinedData); reorder the import block
accordingly (or run isort) so functions/classes like ActionInputs and MinedData
and modules main are in the first-party group and pylint/isort import-order
warnings are resolved.
- Around line 44-54: The fixture reset_action_inputs_cache currently has an
unnecessary type ignore on its signature; remove the " # type: ignore[return]"
from the def reset_action_inputs_cache(...) annotation so the function is simply
declared "def reset_action_inputs_cache(monkeypatch: pytest.MonkeyPatch) ->
None:" and leave the body unchanged, ensuring no other mypy suppressions are
added; reference the fixture name (reset_action_inputs_cache) and the class
ActionInputs in your change.
In `@tests/integration/live/test_bulk_sub_issue_collector.py`:
- Around line 25-36: The smoke test test_bulk_sub_issue_collector_smoke is too
weak; update it to assert that after calling
BulkSubIssueCollector.scan_sub_issues_for_parents the initially requested parent
"AbsaOSS/generate-release-notes#1" appears as a key in
collector.parents_sub_issues and that the value returned by the first call to
scan_sub_issues_for_parents is a list (even if empty). Concretely, call
collector.scan_sub_issues_for_parents once, capture its return into
first_new_parents, assert isinstance(first_new_parents, list), then assert
"AbsaOSS/generate-release-notes#1" in collector.parents_sub_issues (or in
collector.parents_sub_issues.keys()) before doing the second iteration or
further assertions. Ensure you reference BulkSubIssueCollector,
scan_sub_issues_for_parents, and parents_sub_issues in the updated assertions.
In `@tests/integration/test_chapters.py`:
- Around line 294-299: The two assertions collapse because "Direct commit" is a
substring of the heading "Direct commits"; change the test to assert absence of
the heading "### Direct commits" and separately assert absence of a unique
commit message that cannot overlap with that heading (e.g., replace the commit
message used in the test data with something like "Stray commit content" and
assert that string is not in actual), updating the assertions in
tests/integration/test_chapters.py to check both the heading and the unique
record content independently (refer to the variable actual and the existing
heading string "### Direct commits" and update the second assertion to the new
unique message).
In `@tests/integration/test_snapshot.py`:
- Line 33: The test imports build_mined_data and capture_run directly from
conftest.py which is an anti-pattern; move those functions out of conftest into
a regular helper module (e.g., an integration helpers module) and update tests
to import build_mined_data and capture_run from that helper module instead of
conftest.py; optionally re-export the helpers from conftest.py if you still want
them auto-discovered as fixtures/helpers so existing fixtures continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74c97201-1afd-40ec-85d5-83cb48b21e57
📒 Files selected for processing (13)
.github/workflows/test.ymlDEVELOPER.mdrelease_notes_generator/data/miner.pytests/integration/conftest.pytests/integration/fixtures/test_full_pipeline_snapshot.mdtests/integration/live/__init__.pytests/integration/live/test_bulk_sub_issue_collector.pytests/integration/test_chapters.pytests/integration/test_duplicity.pytests/integration/test_filtering_and_routing.pytests/integration/test_record_presentation.pytests/integration/test_release_notes_snapshot.pytests/integration/test_snapshot.py
💤 Files with no reviewable changes (1)
- tests/integration/test_release_notes_snapshot.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/conftest.py (1)
44-54: Avoid resettingActionInputsvia private attributes.This fixture directly patches private members and carries an undocumented
type: ignore. Prefer a public cache-reset helper onActionInputsor a test helper that exercises public APIs only, and remove the ignore if it is not strictly required.Suggested direction
`@pytest.fixture`(autouse=True) -def reset_action_inputs_cache(monkeypatch: pytest.MonkeyPatch) -> None: # type: ignore[return] +def reset_action_inputs_cache(monkeypatch: pytest.MonkeyPatch) -> None: """Reset class-level caches in ActionInputs so each test starts clean.""" - monkeypatch.setattr(ActionInputs, "_row_format_issue", None) - monkeypatch.setattr(ActionInputs, "_row_format_pr", None) - monkeypatch.setattr(ActionInputs, "_row_format_hierarchy_issue", None) - monkeypatch.setattr(ActionInputs, "_row_format_link_pr", None) - monkeypatch.setattr(ActionInputs, "_owner", "") - monkeypatch.setattr(ActionInputs, "_repo_name", "") - monkeypatch.setattr(ActionInputs, "_super_chapters_raw", None) - monkeypatch.setattr(ActionInputs, "_super_chapters_cache", None) + ActionInputs.reset_cached_values()As per coding guidelines: “Must not access private members (names starting with
_) of the class under test directly in tests” and “Avoid type: ignore unless required; document why when used”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 44 - 54, The fixture reset_action_inputs_cache is mutating private ActionInputs attributes directly and uses a type: ignore; instead add a public cache-reset API on ActionInputs (e.g., a classmethod like reset_caches or clear_test_cache) that resets _row_format_issue, _row_format_pr, _row_format_hierarchy_issue, _row_format_link_pr, _owner, _repo_name, _super_chapters_raw, and _super_chapters_cache internally, then update the fixture to call ActionInputs.reset_caches() (no private attribute monkeypatching) and remove the type: ignore; if mocking is still required, monkeypatch the public reset_caches method instead of touching private names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/conftest.py`:
- Around line 259-285: capture_run saves a temp file path into GITHUB_OUTPUT
then unconditionally pops it, which discards any pre-existing environment value;
modify capture_run to first save the original os.environ.get("GITHUB_OUTPUT")
into a local (e.g., old_github_output), set
os.environ["GITHUB_OUTPUT"]=tmp_path, run main.run(), and in the finally block
restore the original by either setting
os.environ["GITHUB_OUTPUT"]=old_github_output if it was not None or deleting the
key only if old_github_output is None; ensure Path(tmp_path).unlink still runs
and no other behavior of capture_run, lines parsing and return remain unchanged.
---
Nitpick comments:
In `@tests/integration/conftest.py`:
- Around line 44-54: The fixture reset_action_inputs_cache is mutating private
ActionInputs attributes directly and uses a type: ignore; instead add a public
cache-reset API on ActionInputs (e.g., a classmethod like reset_caches or
clear_test_cache) that resets _row_format_issue, _row_format_pr,
_row_format_hierarchy_issue, _row_format_link_pr, _owner, _repo_name,
_super_chapters_raw, and _super_chapters_cache internally, then update the
fixture to call ActionInputs.reset_caches() (no private attribute
monkeypatching) and remove the type: ignore; if mocking is still required,
monkeypatch the public reset_caches method instead of touching private names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9e16db3-f446-456a-99c9-61c5e3a45411
📒 Files selected for processing (16)
.github/workflows/test.ymlDEVELOPER.mdrelease_notes_generator/__init__.pyrelease_notes_generator/builder/__init__.pyrelease_notes_generator/chapters/__init__.pyrelease_notes_generator/data/__init__.pyrelease_notes_generator/data/utils/__init__.pyrelease_notes_generator/model/__init__.pyrelease_notes_generator/model/record/__init__.pyrelease_notes_generator/record/__init__.pyrelease_notes_generator/record/factory/__init__.pyrelease_notes_generator/utils/__init__.pytests/integration/conftest.pytests/integration/test_duplicity.pytests/integration/test_filtering_and_routing.pytests/integration/test_snapshot.py
✅ Files skipped from review due to trivial changes (11)
- release_notes_generator/data/utils/init.py
- release_notes_generator/record/factory/init.py
- release_notes_generator/builder/init.py
- release_notes_generator/model/init.py
- release_notes_generator/model/record/init.py
- release_notes_generator/utils/init.py
- release_notes_generator/data/init.py
- release_notes_generator/chapters/init.py
- release_notes_generator/init.py
- release_notes_generator/record/init.py
- DEVELOPER.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/test_snapshot.py
- tests/integration/test_filtering_and_routing.py
tmikula-dev
reviewed
Apr 23, 2026
Collaborator
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
I like the way the Integration test is handled in the project, please react to the comments.
| id: analyze-code | ||
| run: | | ||
| pylint_score=$(pylint $(git ls-files '*.py')| grep 'rated at' | awk '{print $7}' | cut -d'/' -f1) | ||
| pylint_score=$(pylint --ignore-paths='^tests/.*' $(git ls-files '*.py')| grep 'rated at' | awk '{print $7}' | cut -d'/' -f1) |
Collaborator
There was a problem hiding this comment.
Could we have a configuration of pylint inside the .pylintr file? Or is there some reason I can not see to have it here?
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # |
Collaborator
There was a problem hiding this comment.
Please keep the init module level documentation everywhere.
| _super_chapters_cache: list[dict[str, Any]] | None = None | ||
|
|
||
| @staticmethod | ||
| def reset_caches_for_testing() -> None: |
Collaborator
There was a problem hiding this comment.
If this method is used for testing only, should it not be inside the test folder? I am not sure, if it is best separation of concerns.
Comment on lines
+153
to
+168
| ### Run offline tests (no token needed) | ||
|
|
||
| The integration test runs the complete action flow against a real GitHub repository to validate end-to-end functionality. | ||
| ```shell | ||
| pytest tests/integration/ --ignore=tests/integration/live -v | ||
| ``` | ||
|
|
||
| **When it runs:** | ||
| - Automatically on same-repo PRs (NOT on forks for security) | ||
| - Uses the `GITHUB_TOKEN` secret to access the GitHub API | ||
| - Configured in `.github/workflows/test.yml` as the `integration-test-real-api` job | ||
| ### Regenerate golden snapshot | ||
|
|
||
| **What it validates:** | ||
| 1. Action exits with code 0 (success) | ||
| 2. Output contains expected markdown chapter headers (e.g., `### New Features 🎉`) | ||
| 3. Output contains issue/PR references (e.g., `#123`) | ||
| 4. Output contains developer mentions (e.g., `@username`) | ||
| 5. Logs include "completed successfully" message | ||
| 6. Verbose logging is working (DEBUG level found) | ||
| 7. Output contains "Generated release notes:" marker | ||
| ```shell | ||
| WRITE_SNAPSHOTS=1 pytest tests/integration/test_snapshot.py::test_full_pipeline_snapshot | ||
| ``` | ||
|
|
||
| **To run locally** (requires `GITHUB_TOKEN` environment variable): | ||
| ### Live smoke test (requires `GITHUB_TOKEN`) | ||
|
|
||
| ```shell | ||
| export INPUT_TAG_NAME="v0.2.0" | ||
| export INPUT_GITHUB_REPOSITORY="AbsaOSS/generate-release-notes" | ||
| export INPUT_GITHUB_TOKEN="your_github_token" | ||
| export INPUT_CHAPTERS='[{"title": "Features 🎉", "label": "feature"}, {"title": "Bugfixes 🛠", "label": "bug"}]' | ||
| export INPUT_WARNINGS="true" | ||
| export INPUT_PRINT_EMPTY_CHAPTERS="false" | ||
| export INPUT_VERBOSE="true" | ||
| export INPUT_HIERARCHY="false" | ||
| export INPUT_DUPLICITY_SCOPE="both" | ||
| export INPUT_PUBLISHED_AT="false" | ||
| export INPUT_SKIP_RELEASE_NOTES_LABELS="skip-release-notes" | ||
| export PYTHONPATH="${PWD}" | ||
|
|
||
| python main.py | ||
| pytest tests/integration/live/ -v |
Collaborator
There was a problem hiding this comment.
Can you please add like one sentence to every header, so it is clear what for example does the WRITE_SNAPSHOTS shell? Can you also please add some info about why it is benefitial to have offline and live integration tests? It was not clear to me, and it is good to let the user know, when to run which.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
48-52:⚠️ Potential issue | 🟠 MajorKeep
tests/out of the Pylint score.Line 51 currently lints every tracked Python file, including
tests/**/*.py, so the score gate can fail or drift based on test-only code. Use--ignore-pathsor move the setting into.pylintrc.Proposed fix
- pylint_score=$(pylint $(git ls-files '*.py')| grep 'rated at' | awk '{print $7}' | cut -d'/' -f1) + pylint_score=$(pylint --ignore-paths='^tests/.*' $(git ls-files '*.py') | grep 'rated at' | awk '{print $7}' | cut -d'/' -f1)As per coding guidelines, “Lint Python code with Pylint (ignore tests/)”.
#!/bin/bash # Verify whether the current Pylint command would receive test files. # Expected before the fix: test files are listed. # Expected after the fix: command/config excludes tests from scoring. git ls-files '*.py' | rg -n '^tests/' rg -n "pylint .*git ls-files '\\*\\.py'" .github/workflows/test.yml -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 48 - 52, The Pylint invocation in the "Analyze code with Pylint" step (id: analyze-code) currently scores every tracked Python file via the command that sets pylint_score using pylint $(git ls-files '*.py'), which includes tests/; change the command so tests are excluded — e.g. pass a path-ignore to Pylint like --ignore-paths='^tests/' (or configure the same ignore in .pylintrc) when calling pylint so the pipeline computes pylint_score without any tests/*.py files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/helpers.py`:
- Around line 39-54: The helper reset_action_inputs_caches currently mutates
private fields on ActionInputs (e.g. _row_format_hierarchy_issue, _owner,
_super_chapters_cache) — add a public method on the production class (e.g.
ActionInputs.reset_caches() or ActionInputs.clear_test_state()) that
encapsulates resetting those internal caches, update
tests/integration/helpers.py to call that new public method from
reset_action_inputs_caches instead of assigning private attributes, and remove
all direct accesses to ActionInputs._* so the class owns its cache internals.
In `@tests/integration/test_filtering_and_routing.py`:
- Around line 315-323: The test is stubbing get_issues_for_pr to return an empty
set so pr10 isn't seen as linked to a non-closed issue; update the mock for
release_notes_generator.record.factory.default_record_factory.get_issues_for_pr
in the failing test to return a set containing the expected linked issue ID
(e.g., {"pr10-linked-issue"} or the exact issue key used by the test fixtures)
so that the code path in the generator routes pr10 through the "Merged PRs
Linked to 'Not Closed' Issue" branch.
In `@tests/integration/test_record_presentation.py`:
- Around line 80-85: The test stubs out
release_notes_generator.record.factory.default_record_factory.get_issues_for_pr
to return an empty set which makes PRs appear unlinked and hides PR-body
extraction when INPUT_WARNINGS is false; update the mock in
tests/integration/test_record_presentation.py to return the expected issue IDs
for the specific PRs (e.g. {"org/repo#2"} for pr20 and {"org/repo#1"} for pr10)
so capture_run(patch_env, {"INPUT_WARNINGS": "false"}) and the CodeRabbit
assertions exercise the issue↔PR extraction path instead of treating them as
unlinked.
---
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 48-52: The Pylint invocation in the "Analyze code with Pylint"
step (id: analyze-code) currently scores every tracked Python file via the
command that sets pylint_score using pylint $(git ls-files '*.py'), which
includes tests/; change the command so tests are excluded — e.g. pass a
path-ignore to Pylint like --ignore-paths='^tests/' (or configure the same
ignore in .pylintrc) when calling pylint so the pipeline computes pylint_score
without any tests/*.py files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0159742e-bfc6-4be2-b922-efb563abf67d
📒 Files selected for processing (9)
.github/workflows/test.ymlDEVELOPER.mdtests/integration/conftest.pytests/integration/helpers.pytests/integration/test_chapters.pytests/integration/test_duplicity.pytests/integration/test_filtering_and_routing.pytests/integration/test_record_presentation.pytests/integration/test_snapshot.py
✅ Files skipped from review due to trivial changes (1)
- DEVELOPER.md
tmikula-dev
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Created new solution for integration tests.
Release Notes
Related
Closes #306
Summary by CodeRabbit
Tests
Documentation
Chores