Skip to content

test1675: unit tests for URL API helper functions#21296

Closed
bagder wants to merge 11 commits into
masterfrom
bagder/test1675
Closed

test1675: unit tests for URL API helper functions#21296
bagder wants to merge 11 commits into
masterfrom
bagder/test1675

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Apr 13, 2026

  • ipv4_normalize
  • urlencode_str
  • ipv6_parse
  • parse_file

urlapi: make the string URL encoder normalize to uppercase percent-encoding

@testclutch
Copy link
Copy Markdown

Analysis of PR #21296 at d531aa40:

Test 1675 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 62 different CI jobs (the link just goes to one of them).

Test 350 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@bagder bagder requested a review from Copilot April 13, 2026 20:08
@bagder bagder marked this pull request as ready for review April 13, 2026 20:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds/extends URL API unit test coverage for internal URL helper routines and updates URL encoding/file URL parsing behavior to match new expectations (notably: normalize percent-encoding hex to uppercase and reject non-local file://<hostname>/...).

Changes:

  • Add new unit test suite unit1675 covering ipv4_normalize, urlencode_str, ipv6_parse, and parse_file.
  • Update urlencode_str to normalize percent-encoded hex to uppercase and adjust whitespace handling logic.
  • Tighten file:// URL parsing expectations (treat non-local hostnames as CURLUE_BAD_FILE_URL) and update existing tests accordingly.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/unit/unit1675.c New unit test coverage for URL API helper functions.
tests/unit/Makefile.inc Registers unit1675.c in the unit test build.
lib/urlapi.c Exposes helpers for unit tests, normalizes percent-encoding to uppercase, and changes parse_file behavior/signature.
lib/urlapi-int.h Moves internal struct Curl_URL + HOST_* constants into internal header for shared use.
tests/libtest/lib1560.c Updates expected result for non-local file://host... to CURLUE_BAD_FILE_URL.
tests/data/test58 Updates expected request path to uppercase percent-encoding (%5B%5D).
tests/data/test1675 Adds new test definition to run the new unit test.
tests/data/Makefile.am Registers test1675.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/urlapi.c Outdated
Comment thread lib/urlapi.c
Comment thread tests/unit/unit1675.c Outdated
Comment thread tests/unit/unit1675.c
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Apr 13, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Apr 13, 2026

🤖 Augment PR Summary

Summary: This PR expands URL API unit coverage and tightens URL-encoding normalization.

Changes:

  • Introduces new unit test unit1675 (and adds test1675) covering ipv4_normalize, urlencode_str, ipv6_parse, and parse_file.
  • Exposes several formerly-static helpers as UNITTEST functions and adds the required prototypes so they are linkable in unit-test builds.
  • Moves the internal struct Curl_URL definition (and host-type constants) into lib/urlapi-int.h so unit tests can instantiate/use it.
  • Updates urlencode_str() to normalize percent-encoded hex bytes to uppercase (while leaving already-encoded bytes intact, aside from case normalization).
  • Simplifies parse_file() handling by rejecting non-local file://hostname/ URLs and adjusts libtests accordingly.
  • Updates existing test expectations where percent-encoding case changes (e.g. tests/data/test58).

Technical Notes: The encoding normalization is implemented by detecting valid %xx sequences containing lowercase hex digits and re-emitting them via Curl_hexbyte().

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread lib/urlapi.c Outdated
Comment thread tests/unit/unit1675.c Outdated
bagder added 11 commits April 14, 2026 11:27
- ipv4_normalize
- urlencode_str
- ipv6_parse
- parse_file

urlapi: make the string URL encoder normalize to uppercase
percent-encoding
There is no reason we should treat this part different on Windows. Noe
anything except blank, localhost or 127.0.0.1 cause error there as well.
Since we can't use the hostname for anything it is misleading
@bagder bagder requested a review from Copilot April 14, 2026 09:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/urlapi.c
Comment thread tests/unit/unit1675.c
@bagder bagder closed this in 0b4ebeb Apr 14, 2026
bagder added a commit that referenced this pull request Apr 14, 2026
There is no reason we should treat this part different on Windows. Noe
anything except blank, localhost or 127.0.0.1 cause error there as well.

Also: fix query handling in urlencode_str

Closes #21296
@bagder bagder deleted the bagder/test1675 branch April 14, 2026 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants