Feature/182 review of unit tests #192
Conversation
- Added unit tests for bulk_sub_issue_collector logic.
WalkthroughAdds guards around MinedData.since during filtering, refactors DataMiner with factory/worker helpers and safer parallel fetching, centralizes release-note extraction into utils and a Record setup helper, requires Repository in PullRequestRecord construction, updates default record factory, and expands tests for miner/collector/parse logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant Miner as DataMiner
participant BIC as BulkSubIssueCollector
participant GH as GitHub API
participant Worker as __worker / safe_call
CLI->>Miner: mine(data)
Miner->>Miner: derive/guard data.since (may remain None)
alt scan sub-issues
Miner->>Miner: _make_bulk_sub_issue_collector()
Miner->>BIC: scan(parents)
BIC-->>Miner: parent->children map
end
loop parallel fetch missing issues
Miner->>Worker: __worker(parent_id, data, safe_call)
alt __should_fetch true
Worker->>GH: fetch issue (safe_call)
GH-->>Worker: issue or error
Worker-->>Miner: (parent_id, issue, repo, err)
else
Worker-->>Miner: (parent_id, None, None, None)
end
end
Miner->>Miner: attach PRs / update MinedData
Miner-->>CLI: return mined data
sequenceDiagram
autonumber
participant F as filter()
participant D as MinedData
F->>D: inspect data.since
alt data.since is truthy
F->>F: filter PRs by merged_at/closed_at > since
F->>F: filter issues by closed_at > since or keep open
F->>F: filter commits by author_date > since
else
F->>F: deep-copy input without filtering
end
F-->>D: filtered copy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tests/release_notes/data/test_miner.py (6)
254-301: LGTM!Both tests correctly verify the data mining behavior:
test_mine_data_repo_none_raisesproperly bypassessafe_callto surface the rawValueErrortest_mine_data_commits_without_sinceverifies that commits are retrieved without asinceparameter whendata.sinceis NoneOptional: Line 301 could be simplified from
assert getattr(data, "since", None) is Nonetoassert data.since is Nonesincesinceis a standard attribute ofMinedData.
306-326: Remove unused parameter.The
monkeypatchparameter is not used in this test function.Apply this diff:
-def test_scan_sub_issues_for_parents(mocker, mock_repo, mined_data_simple, monkeypatch): +def test_scan_sub_issues_for_parents(mocker, mock_repo, mined_data_simple):
329-356: Remove unused parameter.The
monkeypatchparameter is not used in this test function.Apply this diff:
-def test_fetch_all_repositories_in_cache(mocker, mock_repo, mined_data_simple, monkeypatch): +def test_fetch_all_repositories_in_cache(mocker, mock_repo, mined_data_simple):
359-408: Address static analysis hints for better code quality.Several improvements can be made:
- Line 372: Replace generic
Exceptionwith a more specific exception type (e.g.,RuntimeErrororGithubException).- Line 407: Simplify the verbose slice syntax.
- Remove unused
monkeypatchparameter.Apply this diff:
-def test_fetch_missing_issues(mocker, mock_repo, mined_data_simple, monkeypatch, mock_issue_closed_i1_bug): +def test_fetch_missing_issues(mocker, mock_repo, mined_data_simple, mock_issue_closed_i1_bug): def fake_get_issue(num): if num == 1: return mock_issue_closed_i1_bug if num == 2: i = deepcopy(mock_issue_closed_i1_bug) i.closed_at = None return i if num == 3: i = deepcopy(mock_issue_closed_i1_bug) i.closed_at = datetime(2019, 12, 31) return i if num == 7: - raise Exception("Issue 7 not found") + raise RuntimeError("Issue 7 not found") else: return None # ... rest of the test ... # No additional calls to get_issues assert 1 == len(fetched_issues.keys()) - assert mock_issue_closed_i1_bug == list(fetched_issues.keys())[0] + assert mock_issue_closed_i1_bug == next(iter(fetched_issues.keys())) assert {} == prs_of_fetched_cross_repo_issues
411-435: Remove unused parameters.Both
monkeypatchandmock_issue_closed_i1_bugparameters are not used in this test function.Apply this diff:
-def test_fetch_missing_issues_no_fetch(mocker, mock_repo, mined_data_simple, monkeypatch, mock_issue_closed_i1_bug): +def test_fetch_missing_issues_no_fetch(mocker, mock_repo, mined_data_simple): def fake_get_issue(num): return None
438-484: Simplify attribute assignment.Line 449 uses
setattrwith a constant value, which can be replaced with direct assignment for better readability.Apply this diff:
# Event that should add PR src_issue_with_pr = mocker.Mock() - setattr(src_issue_with_pr, "pull_request", object()) + src_issue_with_pr.pull_request = object() src_issue_with_pr.as_pull_request.return_value = pr_objOtherwise, the test comprehensively covers PR fetching scenarios including error handling and logging verification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
release_notes_generator/data/filter.py(4 hunks)release_notes_generator/data/miner.py(8 hunks)release_notes_generator/data/utils/bulk_sub_issue_collector.py(1 hunks)release_notes_generator/model/mined_data.py(1 hunks)release_notes_generator/model/record/issue_record.py(2 hunks)release_notes_generator/model/record/pull_request_record.py(5 hunks)release_notes_generator/model/record/record.py(3 hunks)release_notes_generator/record/factory/default_record_factory.py(2 hunks)release_notes_generator/utils/record_utils.py(2 hunks)tests/conftest.py(5 hunks)tests/release_notes/data/test_miner.py(3 hunks)tests/release_notes/data/utils/test_bulk_sub_issue_collector.py(1 hunks)tests/release_notes/model/test_issue_record.py(3 hunks)tests/release_notes/model/test_pull_request_record.py(2 hunks)tests/release_notes/record/factory/test_default_record_factory.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T14:09:54.325Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: tests/release_notes/record/factory/test_default_record_factory.py:0-0
Timestamp: 2025-09-28T14:09:54.325Z
Learning: In tests/release_notes/record/factory/test_default_record_factory.py, the mock_get_issues_for_pr_no_issues helper function parameter should remain as `str` type (not changed to `int`) because changing it to `int` causes test failures, even though the parameter is unused and flagged by linters.
Applied to files:
tests/release_notes/record/factory/test_default_record_factory.py
🧬 Code graph analysis (13)
release_notes_generator/model/mined_data.py (1)
release_notes_generator/chapters/base_chapters.py (2)
since(52-61)since(64-65)
release_notes_generator/model/record/pull_request_record.py (3)
release_notes_generator/model/record/record.py (2)
skip(76-78)_get_rls_notes_setup(249-260)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)release_notes_generator/utils/record_utils.py (2)
get_rls_notes_default(72-105)get_rls_notes_code_rabbit(108-157)
release_notes_generator/model/record/record.py (1)
release_notes_generator/action_inputs.py (2)
get_release_notes_title(231-236)is_coderabbit_support_active(239-243)
release_notes_generator/utils/record_utils.py (1)
release_notes_generator/action_inputs.py (1)
get_coderabbit_summary_ignore_groups(255-270)
tests/release_notes/model/test_pull_request_record.py (2)
tests/conftest.py (1)
mock_repo(103-106)release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(18-211)
tests/release_notes/data/test_miner.py (4)
release_notes_generator/data/utils/bulk_sub_issue_collector.py (2)
BulkSubIssueCollector(44-234)scan_sub_issues_for_parents(67-192)release_notes_generator/model/mined_data.py (1)
MinedData(36-80)tests/conftest.py (4)
FakeRepo(48-49)mock_repo(103-106)mined_data_simple(730-743)mock_issue_closed_i1_bug(227-250)release_notes_generator/data/miner.py (4)
DataMiner(45-460)mine_data(54-85)mine_missing_sub_issues(87-108)_fetch_prs_for_fetched_cross_issues(445-460)
tests/release_notes/record/factory/test_default_record_factory.py (1)
tests/conftest.py (1)
mock_repo(103-106)
release_notes_generator/data/miner.py (4)
release_notes_generator/data/utils/bulk_sub_issue_collector.py (2)
BulkSubIssueCollector(44-234)CollectorConfig(21-41)release_notes_generator/action_inputs.py (1)
get_github_token(120-124)release_notes_generator/model/mined_data.py (1)
MinedData(36-80)release_notes_generator/utils/record_utils.py (1)
parse_issue_id(56-64)
tests/release_notes/data/utils/test_bulk_sub_issue_collector.py (1)
release_notes_generator/data/utils/bulk_sub_issue_collector.py (2)
BulkSubIssueCollector(44-234)CollectorConfig(21-41)
release_notes_generator/model/record/issue_record.py (3)
release_notes_generator/utils/record_utils.py (2)
get_rls_notes_default(72-105)get_rls_notes_code_rabbit(108-157)release_notes_generator/model/record/record.py (1)
_get_rls_notes_setup(249-260)release_notes_generator/action_inputs.py (2)
ActionInputs(64-517)get_coderabbit_release_notes_title(246-252)
release_notes_generator/record/factory/default_record_factory.py (2)
release_notes_generator/utils/pull_request_utils.py (2)
get_issues_for_pr(56-81)extract_issue_numbers_from_body(32-52)release_notes_generator/model/record/pull_request_record.py (1)
register_commit(179-186)
tests/release_notes/model/test_issue_record.py (2)
release_notes_generator/model/record/issue_record.py (6)
issue(93-98)get_pull_request_numbers(214-220)get_commit(199-212)get_pull_request(187-197)register_commit(232-246)register_pull_request(222-230)release_notes_generator/utils/record_utils.py (1)
get_rls_notes_code_rabbit(108-157)
tests/conftest.py (2)
release_notes_generator/model/mined_data.py (1)
MinedData(36-80)release_notes_generator/model/record/pull_request_record.py (1)
PullRequestRecord(18-211)
🪛 GitHub Actions: Build and Test
release_notes_generator/model/record/record.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by black; run 'black' to format and re-run the check. Command: black --check $(git ls-files '*.py')
🪛 Ruff (0.13.3)
release_notes_generator/utils/record_utils.py
144-144: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
tests/release_notes/data/test_miner.py
306-306: Unused function argument: monkeypatch
(ARG001)
329-329: Unused function argument: monkeypatch
(ARG001)
359-359: Unused function argument: monkeypatch
(ARG001)
372-372: Create your own exception
(TRY002)
372-372: Avoid specifying long messages outside the exception class
(TRY003)
407-407: Prefer next(iter(fetched_issues.keys())) over single element slice
Replace with next(iter(fetched_issues.keys()))
(RUF015)
411-411: Unused function argument: monkeypatch
(ARG001)
411-411: Unused function argument: mock_issue_closed_i1_bug
(ARG001)
412-412: Unused function argument: num
(ARG001)
449-449: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
release_notes_generator/data/miner.py
193-193: Do not catch blind exception: Exception
(BLE001)
247-247: Do not catch blind exception: Exception
(BLE001)
258-258: Do not catch blind exception: Exception
(BLE001)
457-457: Do not catch blind exception: Exception
(BLE001)
tests/release_notes/data/utils/test_bulk_sub_issue_collector.py
16-16: Avoid specifying long messages outside the exception class
(TRY003)
38-38: Avoid specifying long messages outside the exception class
(TRY003)
60-60: Possible hardcoded password assigned to argument: "token"
(S106)
186-186: Unused function argument: monkeypatch
(ARG001)
tests/release_notes/model/test_issue_record.py
81-81: Unused function argument: issue_record
(ARG001)
88-88: Unused function argument: issue_record
(ARG001)
101-101: Unused function argument: issue_record
(ARG001)
120-120: Unused function argument: issue_record
(ARG001)
🔇 Additional comments (19)
release_notes_generator/model/mined_data.py (1)
44-45: LGTM! Intentional change to support optional filtering.The change from a default epoch datetime to
Nonealigns with the new guarded filtering logic infilter.py, wheredata.sinceis checked before applying date-based filters. WhensinceisNone, the data is deep-copied without filtering.release_notes_generator/data/filter.py (4)
82-91: LGTM! Guard prevents filtering when since is None.The addition of
data.since andensures that PR filtering only occurs when a since timestamp is provided, aligning with the new optional filtering behavior.
169-173: Clarify issue filtering behavior whensinceisNone
Both default and hierarchy filters currently include only open issues whendata.sinceis unset, excluding all closed issues. Confirm if omittingsinceshould disable filtering entirely (including closed issues) and adjust the conditions if needed.
93-98: Include all commits whensinceis None
Inrelease_notes_generator/data/filter.py(lines 93–98), update the comprehension to:commits_dict = { commit: repo for commit, repo in data.commits.items() - if data.since and commit.commit.author.date > data.since + if not data.since or commit.commit.author.date > data.since }Without this change, all commits are excluded whenever
data.sinceisNone. Confirm this aligns with the intended behavior.
151-155: Ignore since=None filter suggestion:FilterByRelease.filteronly runs whendata.releaseis non-null, and the miner always setsdata.sincewhen a release exists, so the(data.since and …)branch never seesNone. No change required.Likely an incorrect or invalid review comment.
release_notes_generator/data/utils/bulk_sub_issue_collector.py (1)
216-216: LGTM! Appropriate use of pylint disable annotation.The
broad-exception-caughtdisable is appropriate here since the retry logic intentionally catches all exceptions to enable retry attempts before re-raising.tests/release_notes/record/factory/test_default_record_factory.py (2)
178-178: LGTM! Proper convention for unused variables.Prefixing unused variables with underscores is a Python convention that clearly indicates these values are intentionally not used in the test, improving code readability.
270-270: LGTM! Consistent unused variable naming.Same improvement as line 178, properly indicating intentionally unused variables.
release_notes_generator/model/record/record.py (1)
249-260: LGTM! Centralized setup method reduces duplication.The new
_get_rls_notes_setupmethod consolidates the detection regex compilation and configuration retrieval, which will be used by subclasses. This improves maintainability by centralizing the setup logic.release_notes_generator/model/record/issue_record.py (1)
164-183: LGTM! Effective refactoring to utility functions.The extraction of release notes logic to external utilities (
get_rls_notes_defaultandget_rls_notes_code_rabbit) and the use of_get_rls_notes_setupfor centralized configuration improves code organization and reduces duplication across record types.release_notes_generator/record/factory/default_record_factory.py (2)
134-136: LGTM! Cleaner set construction.Merging the linked issues in a single statement using
updateis more concise than the previous separate construction and union approach.
168-171: LGTM! Repository context properly passed to PullRequestRecord.The changes correctly pass the repository context to
PullRequestRecord(line 168) and consistently reference the stored record when registering commits (line 170), aligning with the updated constructor signature.tests/release_notes/model/test_pull_request_record.py (1)
23-39: LGTM! Test updated to match new constructor signature.The test correctly adds
mock_repoas a parameter and passes it to thePullRequestRecordconstructor, aligning with the production changes that now require a repository context.tests/release_notes/data/test_miner.py (2)
36-53: LGTM!The test double appropriately inherits from the real class to satisfy
isinstancechecks while avoiding network calls. The implementation correctly initializes the parent class and provides controllable test behavior.
96-251: LGTM!The test suite comprehensively covers various scenarios for
get_latest_release:
- Error handling (TypeError, ValueError)
- Happy path with releases
- No releases found
- Tag name defined/not defined cases
The tests properly mock dependencies and verify both behavior and log messages.
tests/conftest.py (4)
16-16: LGTM!Removing the unused
copyimport and keeping onlydeepcopyis appropriate for this codebase.
48-49: LGTM!The
FakeRepohelper class is appropriately minimal for testing purposes and is used intest_miner.pyfor simulating repository objects.
729-743: LGTM!The
mined_data_simplefixture provides clean, minimal test data for testing DataMiner functionality. The fixture correctly:
- Initializes MinedData with a mock repository
- Creates a single closed issue with release notes
- Sets up empty collections for pull requests and commits
1136-1181: LGTM!All pull request record fixtures have been correctly updated to accept and pass the
mock_repoparameter toPullRequestRecord, aligning with the production code changes wherePullRequestRecord.__init__now requires arepoparameter.
| def _make_bulk_sub_issue_collector(self) -> BulkSubIssueCollector: | ||
| cfg = CollectorConfig(verify_tls=False) | ||
| return BulkSubIssueCollector(ActionInputs.get_github_token(), cfg) | ||
|
|
There was a problem hiding this comment.
Do not disable TLS verification for GraphQL requests
Setting verify_tls=False on the default collector forces every production GraphQL call to skip certificate verification, which is a critical security regression. Please keep TLS verification enabled (and, if needed, make the override opt-in for tests only).
- cfg = CollectorConfig(verify_tls=False)
- return BulkSubIssueCollector(ActionInputs.get_github_token(), cfg)
+ cfg = CollectorConfig()
+ return BulkSubIssueCollector(ActionInputs.get_github_token(), cfg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _make_bulk_sub_issue_collector(self) -> BulkSubIssueCollector: | |
| cfg = CollectorConfig(verify_tls=False) | |
| return BulkSubIssueCollector(ActionInputs.get_github_token(), cfg) | |
| def _make_bulk_sub_issue_collector(self) -> BulkSubIssueCollector: | |
| - cfg = CollectorConfig(verify_tls=False) | |
| cfg = CollectorConfig() | |
| return BulkSubIssueCollector(ActionInputs.get_github_token(), cfg) |
🤖 Prompt for AI Agents
In release_notes_generator/data/miner.py around lines 110 to 113, the
BulkSubIssueCollector is being created with TLS verification disabled
(verify_tls=False) which is a security regression; restore TLS verification by
removing the forced verify_tls=False (or set it to True) so production GraphQL
calls validate certificates, and if tests need to disable TLS make the override
opt-in via a clearly named test-only flag or environment variable (e.g.,
READONLY_TEST_DISABLE_TLS) or add an optional parameter on the miner that tests
can pass; update any tests that relied on the current behavior to explicitly opt
into the test-only override.
There was a problem hiding this comment.
There is decision to keep it like it is, for now.
There was a problem hiding this comment.
@miroslavpojer, understood. I've noted your decision to keep the TLS verification disabled for now.
Would you like me to create a follow-up issue to track re-enabling TLS verification as a future improvement?
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#192
File: release_notes_generator/data/miner.py:110-113
Timestamp: 2025-10-08T10:54:57.767Z
Learning: In release_notes_generator/data/miner.py, the verify_tls=False parameter in the CollectorConfig for BulkSubIssueCollector should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#168
File: release_notes_generator/utils/pull_request_utils.py:69-70
Timestamp: 2025-09-24T07:24:03.693Z
Learning: In release_notes_generator/utils/pull_request_utils.py, the verify=False parameter in the requests.post call for GitHub API access should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release_notes/data/test_miner.py (1)
118-122: Fix log assertion indexLine 121 indexes
mock_log_error.call_args_list[2], but with only two releases under test the list has length 2, so this blows up withIndexErrorand the suite fails. Please switch the second check to index 1 so the assertion targets the second log entry.- assert ('Skipping invalid type of version tag: %s | Error: %s', 'v2.0.0', '') == mock_log_error.call_args_list[2][0] + assert ('Skipping invalid type of version tag: %s | Error: %s', 'v2.0.0', '') == mock_log_error.call_args_list[1][0]
🧹 Nitpick comments (1)
release_notes_generator/model/record/record.py (1)
198-199: Consider making the TODO comment more descriptive.While referencing issue #191 is helpful, briefly describing what needs to be fixed would improve code readability without requiring developers to look up the issue.
Example:
- # TODO - fix in #191 + # TODO (#191): Review thread-safety of counter increment or clarify single-threaded assumption
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
release_notes_generator/model/record/record.py(3 hunks)tests/release_notes/data/test_miner.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/release_notes/data/test_miner.py (4)
release_notes_generator/data/utils/bulk_sub_issue_collector.py (2)
BulkSubIssueCollector(44-234)scan_sub_issues_for_parents(67-192)release_notes_generator/model/mined_data.py (1)
MinedData(36-80)tests/conftest.py (3)
FakeRepo(48-49)mock_repo(103-106)mined_data_simple(730-743)release_notes_generator/data/miner.py (1)
_fetch_prs_for_fetched_cross_issues(445-460)
release_notes_generator/model/record/record.py (1)
release_notes_generator/action_inputs.py (2)
get_release_notes_title(231-236)is_coderabbit_support_active(239-243)
🪛 Ruff (0.13.3)
tests/release_notes/data/test_miner.py
372-372: Avoid specifying long messages outside the exception class
(TRY003)
411-411: Unused function argument: mock_issue_closed_i1_bug
(ARG001)
412-412: Unused function argument: num
(ARG001)
🔇 Additional comments (1)
release_notes_generator/model/record/record.py (1)
22-22: LGTM! Imports are correctly used.The
reandActionInputsimports are properly utilized in the new_get_rls_notes_setup()helper method.Also applies to: 26-27
1 participant
Release Notes:
Summary by CodeRabbit
New Features
Bug Fixes
Reliability
Tests
Fixes #182