Skip to content

chore: improve fastwebsockets maintenance path#134

Closed
oab24413gmai wants to merge 1 commit into
denoland:mainfrom
oab24413gmai:maint/20260524211713
Closed

chore: improve fastwebsockets maintenance path#134
oab24413gmai wants to merge 1 commit into
denoland:mainfrom
oab24413gmai:maint/20260524211713

Conversation

@oab24413gmai

Copy link
Copy Markdown

Summary:

  • Add edge-case unit tests in src/mask.rs for the unmask routine covering zero-length payloads, length-1, length-3 (sub-word), and length spanning a 4-byte boundary plus tail bytes, asserting round-trip mask/unmask equivalence and that the mask key indexing matches RFC 6455 §5.3. Optionally add a frame-roundtrip test in src/frame.rs for a payload at the 125/126 length boundary to exercise the 16-bit length encoding path.
  • Keep the change narrow so it is straightforward to review.

Notes:

  • I kept this scoped to the relevant implementation and tests.

@divybot

divybot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Maintenance scan note: CI is failing only at the formatting step after build and tests passed. The workflow runs cargo fmt -- --check --verbose on all platforms, so this should just need cargo fmt on the branch and a push.

Once formatting is fixed, this looks like a narrow test-only PR; no need to rerun anything broader than the existing CI unless the patch changes beyond formatting.

@divybot

divybot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Maintenance scan note: CI failed on the Check formatting step. cargo fmt --check reports three diffs in the new tests in src/frame.rs:

  • around line 385: the 127 => arm's let len = u64::from_be_bytes(buf[2..10].try_into().unwrap()) as usize; should fit on one line.
  • around line 393: the mask-key array should be vertical-formatted —
    let m = [
      buf[offset],
      buf[offset + 1],
      buf[offset + 2],
      buf[offset + 3],
    ];
  • around line 414: let (fin, opcode, mask, payload_len, offset) = parse_frame_header(&buf); should fit on one line.

Running cargo fmt locally and pushing should clear all three jobs. Ubuntu and Windows were cancelled by the macOS failure rather than failing independently, so a clean cargo fmt is likely the entire fix.

The test shapes themselves — 125/126 length-encoding boundary and the length-0/1/3/7 mask cases — look like reasonable gap-fillers.

@littledivy littledivy 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.

Thank you, we have plenty tests and this PR fixes no bug

@littledivy littledivy closed this Jun 2, 2026
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.

3 participants