Skip to content

test: restore abspath after include-order tests - #473

Open
yangfan-yf-yf wants to merge 1 commit into
cpplint:developfrom
yangfan-yf-yf:fix/include-order-test-path-isolation
Open

yangfan-yf-yf wants to merge 1 commit into
cpplint:developfrom
yangfan-yf-yf:fix/include-order-test-path-isolation

Conversation

@yangfan-yf-yf

@yangfan-yf-yf yangfan-yf-yf commented Sep 14, 2026

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.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

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.

@walidbi200 walidbi200 left a comment

Copy link
Copy Markdown

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.

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