Fix deque module name for test_repr #5799
Conversation
WalkthroughThe updates adjust test expectations and module associations for the Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
flaky or something changed? |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Lib/test/test_httplib.py (1)
183-186: 💡 Verification agent🧩 Analysis chain
Stacking
expectedFailure+skipIfmay mask future fixes
The test is now wrapped asexpectedFailure(skipIf(test)), meaning:
• on macOS it is skipped,
• on other platforms it still expects to fail.If the underlying bug eventually gets fixed, the expected-failure wrapper will silently swallow the success everywhere except macOS, so the CI will not alert you.
Two alternatives:-@unittest.expectedFailure -@unittest.skipIf(sys.platform == 'darwin', 'Occasionally success on macOS') +@unittest.skipIf(sys.platform == 'darwin', + 'Flaky on macOS – see issue ####') +# Drop expectedFailure once the root cause is fixed everywhere—or— drop
expectedFailureentirely and rely solely onskipIfuntil the flakiness is understood.Either way the intent becomes clearer and future green runs will surface the fix.
I’d like to confirm how the decorators are applied in
Lib/test/test_httplib.pyto ensure the advice is accurate. Let’s pull in the surrounding lines for both decorators:
🏁 Script executed:
#!/bin/bash # Show skipIf decorator and its context rg -n -A2 "@unittest\.skipIf" Lib/test/test_httplib.py # Show expectedFailure decorator and its context rg -n -A2 "@unittest\.expectedFailure" Lib/test/test_httplib.pyLength of output: 1146
Refactor
test_local_unknown_certdecorator usageThe test is currently decorated as:
@unittest.expectedFailure @unittest.skipIf(sys.platform == 'darwin', 'Occasionally success on macOS') def test_local_unknown_cert(self): # The custom cert isn't known to the default trust bundle …This means
- macOS: test is skipped
- other platforms: failures are expected (and any passes become “unexpected success” without failing CI)
If the underlying issue is fixed, CI won’t report a green build. To ensure a future pass surfaces as a real success, replace the stacked decorators with a single
skipIf(and drop theexpectedFailure):-@unittest.expectedFailure -@unittest.skipIf(sys.platform == 'darwin', 'Occasionally success on macOS') +@unittest.skipIf( + sys.platform == 'darwin', + 'Flaky on macOS – see issue ####' +) def test_local_unknown_cert(self): # The custom cert isn't known to the default trust bundle …Once the root cause is resolved everywhere, you can remove the
skipIfas well.
🧹 Nitpick comments (1)
Lib/test/test_httplib.py (1)
1-1:sysimport looks fine but placement is non-alphabetical
Nothing functionally wrong, yetsysends up as the very first import while other std-lib imports are alphabetically ordered (array,errno, …). If you follow CPython’s own style guides, consider re-ordering to keep import blocks sorted to avoid future merge noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Lib/test/test_dataclasses.py(0 hunks)Lib/test/test_httplib.py(2 hunks)Lib/test/test_reprlib.py(0 hunks)vm/src/stdlib/collections.rs(1 hunks)
💤 Files with no reviewable changes (2)
- Lib/test/test_reprlib.py
- Lib/test/test_dataclasses.py
🚧 Files skipped from review as they are similar to previous changes (1)
- vm/src/stdlib/collections.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
vm/src/codecs.rs (1)
408-414: Apply the same “return‐less” pattern to the remaining early exits for stylistic consistencyThe newly introduced
_ => Nonebranch follows the implicit-return style, but the earlierreturn Some(Self::UTF_16_NE);(line 406) andreturn Some(Self::UTF_32_NE);(line 416) still use an explicitreturn.
Replacing those with plainSome(...)would keep the entire matcher uniform and a bit more idiomatic.vm/src/stdlib/sys.rs (1)
726-727: Minor style nit: unnecessary:?
thresholdis ausize; Debug ({:?}) and Display ({}) output are identical.
Consider plain{threshold}for terser code.- let error = format!("maxdigits must be 0 or larger than {threshold:?}"); + let error = format!("maxdigits must be 0 or larger than {threshold}");vm/src/exceptions.rs (2)
207-210: Minor consistency nit – mix of formatting styles in the same blockThe new inline interpolation (
format!(" ({filename})")) is perfectly fine, but two lines above the code still uses the traditional positional style (r##" File "{}", line {}"##).
For readability, consider sticking to one style within the same logical block (or whole file).
212-219: Same consistency point for the source-line write
writeln!(output, " {l_text}")?;adopts the 1.58-style interpolation, while neighbouring statements keep the old style.
Not critical, but aligning on a single convention makes future reviews easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/lib.rs(1 hunks)src/shell.rs(1 hunks)stdlib/src/binascii.rs(1 hunks)stdlib/src/csv.rs(2 hunks)vm/src/buffer.rs(2 hunks)vm/src/builtins/complex.rs(1 hunks)vm/src/builtins/object.rs(1 hunks)vm/src/builtins/property.rs(1 hunks)vm/src/builtins/super.rs(2 hunks)vm/src/codecs.rs(2 hunks)vm/src/eval.rs(1 hunks)vm/src/exceptions.rs(3 hunks)vm/src/frame.rs(1 hunks)vm/src/protocol/number.rs(4 hunks)vm/src/stdlib/ctypes/base.rs(1 hunks)vm/src/stdlib/ctypes/function.rs(1 hunks)vm/src/stdlib/ctypes/structure.rs(1 hunks)vm/src/stdlib/io.rs(1 hunks)vm/src/stdlib/posix.rs(2 hunks)vm/src/stdlib/sys.rs(2 hunks)vm/src/vm/compile.rs(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- vm/src/eval.rs
- vm/src/stdlib/ctypes/structure.rs
- src/shell.rs
- vm/src/vm/compile.rs
- vm/src/builtins/object.rs
- vm/src/stdlib/ctypes/base.rs
- stdlib/src/csv.rs
- vm/src/protocol/number.rs
- vm/src/buffer.rs
- vm/src/frame.rs
- vm/src/builtins/property.rs
- src/lib.rs
- vm/src/stdlib/ctypes/function.rs
- vm/src/stdlib/posix.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (8)
vm/src/codecs.rs (1)
1117-1120: LGTM – dropped the superfluousreturnThe else-branch now just evaluates to
Err(bad_err_type(err, vm)), matching the style of the preceding branches. No behavioural change, compilation is unaffected.stdlib/src/binascii.rs (1)
759-760: LGTM – modernized format stringUsing inline
{length}is clearer and keeps the message identical at run-time.vm/src/builtins/complex.rs (1)
69-72: LGTM – warning text refactorInterpolation via
{ret_class}matches Rust ≥ 1.58 idioms and doesn’t change behaviour.
No further issues spotted.vm/src/stdlib/sys.rs (1)
320-323: Good simplification of error propagationInline
{e}removes an unnecessary positional argument – no logic change.
Looks fine.vm/src/builtins/super.rs (2)
1-1: Comment addition is harmless
cspelldirective only affects tooling; no runtime impact.
129-131: Clean write-lock assignment simplifies codeDirectly overwriting
*zelf.inner.write()is clearer and avoids the now-superfluousmem::swap.
No concurrency or safety regressions detected.vm/src/stdlib/io.rs (1)
1161-1180: Change looks good – direct assignment is clearer than aswap.Replacing the previous
std::mem::swapwith*v = read_buf.take();makes the intent obvious (restore the taken buffer) and avoids the extra move incurred by
swap. Functionality remains identical and consistent with the analogous write-path a few lines above (self.buffer = write_buf.take();).No further issues spotted in this hunk.
vm/src/exceptions.rs (1)
1614-1619: Double-check Display impls formsgandlineno
format!("{msg} (line {lineno})")relies onPyStrRefimplementingDisplay.
That is currently true, but if this ever changes the compile error will only show up here.
Explicitly converting with.as_str()would make the intent obvious and decouple the code from that impl detail.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Lib/test/test_httplib.py (1)
1785-1786: Decorator order & platform match – consider a minor tweakThe stack
@unittest.expectedFailure @unittest.skipIf(sys.platform == 'darwin', 'Occasionally success on macOS')works, but it first wraps the function with
skipIf, then withexpectedFailure.
For clarity (and to avoid the double-wrapper calling anexpectedFailurewrapper that never actually runs on macOS), it’s slightly cleaner to putskipIfoutermost:-@unittest.expectedFailure -@unittest.skipIf(sys.platform == 'darwin', 'Occasionally success on macOS') +@unittest.skipIf(sys.platform == 'darwin', 'Occasionally success on macOS') +@unittest.expectedFailureResult:
• On macOS the test is skipped immediately.
• On other platforms the test still registers as an expected failure.Purely cosmetic – behaviour is unchanged, so feel free to ignore if you prefer the current order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Lib/test/test_dataclasses.py(0 hunks)Lib/test/test_httplib.py(2 hunks)Lib/test/test_reprlib.py(0 hunks)vm/src/stdlib/collections.rs(1 hunks)
💤 Files with no reviewable changes (2)
- Lib/test/test_reprlib.py
- Lib/test/test_dataclasses.py
✅ Files skipped from review due to trivial changes (1)
- vm/src/stdlib/collections.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
Lib/test/test_httplib.py (1)
1-1:sysimport added in isolation looks fineThe extra
import sysis required for the new@unittest.skipIf()condition and does not clash with any other symbols in this file.
Summary by CodeRabbit
Bug Fixes
Refactor