GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test: restore abspath after include-order tests by yangfan-yf-yf · Pull Request #473 · cpplint/cpplint

Conversation

yangfan-yf-yf commented Sep 14, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

I found that TestOrderOfIncludes replaces os.path.abspath with an identity function, but the inherited tearDown() is not invoked for this plain pytest class. The replacement therefore leaks into later tests: FileInfo("source.cc").FullName() returns "source.cc" instead of an absolute path.

Use the monkeypatch fixture to restore the original function after each include-order test. Add a FileInfo regression in a real working directory and a changelog entry. The include-order tests keep their existing mocked-path behavior; production code is unchanged.

Validation on Windows / Python 3.12.3:

Before the fixture fix, the new path test passes alone, but running TestOrderOfIncludes followed by TestFileInfo gives 13 passed, 1 failed.
After the fix, the same combination gives 14 passed.
Reproduction command: python -m pytest --no-cov cpplint_unittest.py::TestOrderOfIncludes cpplint_unittest.py::TestFileInfo.
Full suite: 232 passed, coverage 96.46% (required: 95%).
Pylint and Mypy passed.
Pre-commit checks on both changed files passed, including Ruff check/format and Codespell; git diff --check passed.
CI run 34807125497 passed for this head combined with develop: Ubuntu, macOS, and Windows, each on Python 3.10 and 3.14. All six jobs ran 232 tests successfully, with 96.46% coverage, and passed Pylint and Mypy. The remote pre-commit check also passed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          test: restore abspath after include-order tests

1be4546

coderabbitai Bot commented Sep 14, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: dd2dabf3-1a0b-4d5a-b0ee-8c2e98533f5d

📥 Commits

Reviewing files that changed from the base of the PR and between 8628446 and 1be4546.

📒 Files selected for processing (2)

CHANGELOG.rst
cpplint_unittest.py

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

The change scopes the include-order test’s os.path.abspath override, adds coverage for FileInfo.FullName() with a temporary working directory, and documents the test isolation update.

Changes

Test isolation and path resolution

Layer / File(s)	Summary
Isolate path overrides and test absolute paths `cpplint_unittest.py`, `CHANGELOG.rst`	`TestOrderOfIncludes` uses `monkeypatch` to restore the `abspath` override after each test. `TestFileInfo` verifies absolute-path resolution. The changelog records the isolation change.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Suggested reviewers: aaronliu0130

Merge Risk: ⚪ Minimal · up to 1be45

The change is limited to test isolation and coverage, with no unresolved merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: restoring os.path.abspath after the include-order tests.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. (1 skipped: 1 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

walidbi200 approved these changes

Sep 17, 2026

View reviewed changes

walidbi200 left a comment

Copy link

Copy Markdown

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.

Reviewed current head 1be4546. I reproduced the os.path.abspath leak on the base commit: after TestOrderOfIncludes, the patched function remained in place and FileInfo("source.cc").FullName() returned the relative path.

On this head, the same test sequence restores the original os.path.abspath, and FullName() resolves to an absolute path. The focused regression gives 14/14 passing, and I also ran the full suite (232 passed, 96.46% coverage), pylint, mypy, pre-commit, and git diff --check successfully.

The monkeypatch fixture scopes the existing test override correctly and production code is untouched. LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bensynapse mentioned this pull request

Oct 8, 2026

Looking for more maintainers #402

Open

PNHD approved these changes

Oct 20, 2026

View reviewed changes

PNHD left a comment

Copy link

Copy Markdown

Contributor

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.

Reviewed head 1be454680df8dd9239ee1a7fa4a12c8b4e93df89.

The change scopes the existing os.path.abspath override to the requesting tests
while preserving their mocked-path behavior. The new FileInfo regression checks
the observable consequence in a real working directory, and production
cpplint.py is unchanged from the base.

I independently reproduced the cause on Python 3.12.14: executing the base
include-order fixture body leaves os.path.abspath replaced after it returns, so
FileInfo("source.cc").FullName() becomes the relative string "source.cc".
Without that leaked override it resolves to the expected absolute path.

For clarity, that was a direct Python reproduction of the base fixture body,
not a local pytest run. Pytest is unavailable in this environment. I also checked
Actions run 34807125497: all six platform/Python jobs and their Test with tox
steps completed successfully.

The function-scoped monkeypatch cleanup addresses the leak, and I found no
blocking issue in this diff.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cclauss added the git conflict must be resolved label

Nov 5, 2026

DanceoftheAurora commented Nov 8, 2026

Copy link

Copy Markdown

I prepared an optional integration of this PR with current develop (880b36e) in c088c0d, on fix/pr473-merge-develop.

Both parents are preserved. The only manual conflict resolution retains both CHANGELOG entries; the original fixture isolation and FileInfo regression are unchanged, and production cpplint.py is byte-identical to develop. Local macOS/Python 3.12.14 validation: 242 tests passed (96.66% coverage), Pylint, Mypy, and applicable pre-commit hooks passed. This integration was AI-assisted; no hosted CI result is claimed for this new commit.

The original author can merge this branch if useful; it adds no production behavior beyond current develop.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments

Reviewers

2 more reviewers

PNHD

PNHD approved these changes

walidbi200

walidbi200 approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

git conflict must be resolved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

yangfan-yf-yf commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

walidbi200 left a comment

Choose a reason for hiding this comment

Uh oh!

PNHD left a comment

Choose a reason for hiding this comment

Uh oh!

DanceoftheAurora commented Nov 8, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yangfan-yf-yf commented Sep 14, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Sep 14, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.