Conversation
WalkthroughUpdated dependency pins and test infra: vcpkg revision in Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test runner
participant WaitForPort as wait_for_port()
participant PortCheck as OS/network
participant Browser as WebDriver / Browser
TestRunner->>WaitForPort: call wait_for_port(port, host="localhost")
WaitForPort->>PortCheck: probe localhost:port (loop until ready or timeout)
alt port becomes available
PortCheck-->>WaitForPort: success
WaitForPort-->>TestRunner: return
TestRunner->>Browser: load http://localhost:PORT/
Browser-->>TestRunner: page loaded / runtime reachable
else timeout
PortCheck-->>WaitForPort: timeout/error
WaitForPort-->>TestRunner: raise/return failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (4)
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 |
ShaharNaveh
force-pushed
the
update-vcpkg
branch
from
3fe5b4d to
7cb0402
Compare
|
@youknowone mind helping here? I thought it was the selenium version, but it's not. I ain't so sure why it tries to connect to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
wasm/tests/conftest.py (1)
98-98: Critical: Update hardcoded0.0.0.0tolocalhostto match the port-wait logic.The
wait_for_port(PORT)call at line 32 now checkslocalhost:8080(per the updated default at line 45), but the browser still attempts to connect tohttp://0.0.0.0:8080. In many CI environments,0.0.0.0is not a valid connection target from the browser context—it's a bind address. This mismatch likely causes the CI failures mentioned in the PR.Apply this diff to align the browser connection with the port-wait logic:
- driver.get(f"http://0.0.0.0:{PORT}") + driver.get(f"http://localhost:{PORT}")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wasm/tests/conftest.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (2)
wasm/tests/conftest.py (2)
1-8: LGTM! Import additions support the functionality below.The added imports (
atexit,time,pytest) are all utilized in the file and the reordering improves clarity.
69-73: LGTM! Selenium imports are properly organized.The addition of
JavascriptExceptionand reordering of other imports align with their usage in the test fixtures below.
ShaharNaveh
force-pushed
the
update-vcpkg
branch
from
f58620f to
ef4f4c0
Compare
|
awesome, you found it |
|
Can we merge? :) |
2 participants
Summary by CodeRabbit