Skip to content

Fix Failing Tests, Address Client Connection Issue#155

Open
jhoughjr wants to merge 8 commits into
vapor:mainfrom
jhoughjr:main
Open

Fix Failing Tests, Address Client Connection Issue#155
jhoughjr wants to merge 8 commits into
vapor:mainfrom
jhoughjr:main

Conversation

@jhoughjr

Copy link
Copy Markdown

Updated from swift-tools-version:5.7 to 5.9.
This fixes an issue where clients would get a bad Sec-WebSocket-Accept value from vapor.

Fixed bad url tests on WebSocket, Async and regular.

There is an issue for covering all bad urls that the WebSocket might reject as URL seems to init fragments such as bare escape sequences. I'm not sure yet how that could be covered better, or how important it is.

@codecov

codecov Bot commented Nov 22, 2024

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.85%. Comparing base (a935b63) to head (2859454).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #155   +/-   ##
=======================================
  Coverage   82.85%   82.85%           
=======================================
  Files           6        6           
  Lines         659      659           
=======================================
  Hits          546      546           
  Misses        113      113           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gwynne

gwynne commented Nov 22, 2024

Copy link
Copy Markdown
Member

The semantics of URL's validation changed noticeably in Swift 6.0, hence the CI failure.

@0xTim

0xTim commented Nov 22, 2024

Copy link
Copy Markdown
Member

We really shouldn't be touching BoringSSL anymore

@gwynne

gwynne commented Nov 22, 2024

Copy link
Copy Markdown
Member

We really shouldn't be touching BoringSSL anymore

Yeah, but that's out of scope for this PR.

@jhoughjr

Copy link
Copy Markdown
Author

Ill check these tests and update.

@jhoughjr

Copy link
Copy Markdown
Author

Im beginning to think there is no way to resolve this issue with this test. The behavior is not consistent enough across swift versions and platforms. The utility of the test is unknown to me also.

I don't know how to even do it (much less elegantly), to handle URL initialization across versions and platforms.
I'd also argue this is kinda testing URL and we don't own that. idk im not of much use.

@0xTim 0xTim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realise this was building! @gwynne any comments on this?

@0xTim 0xTim added the semver-minor Contains new APIs label Mar 21, 2025
@hiSandog

hiSandog commented Jul 4, 2026

Copy link
Copy Markdown

For the client connection test fix, it would be useful to include a failure-path case as well as the happy path. A local server that accepts the TCP connection and then closes during the WebSocket upgrade would prove the client cleans up futures/channels deterministically. That is usually where flaky tests hide, because success paths complete quickly while failed handshakes can leave event-loop work pending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor Contains new APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants