test: restore abspath after include-order tests - #473
yangfan-yf-yf wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change scopes the include-order test’s ChangesTest isolation and path resolution
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ 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 |
walidbi200
left a comment
There was a problem hiding this comment.
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.
I found that
TestOrderOfIncludesreplacesos.path.abspathwith an identity function, but the inheritedtearDown()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
monkeypatchfixture to restore the original function after each include-order test. Add aFileInforegression 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:
TestOrderOfIncludesfollowed byTestFileInfogives 13 passed, 1 failed.python -m pytest --no-cov cpplint_unittest.py::TestOrderOfIncludes cpplint_unittest.py::TestFileInfo.git diff --checkpassed.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.