✅ add qs.js comparison #5
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughCentralizes Xcode/Swift versions in CI, simplifies macOS runners, and adds a deterministic Swift↔︎qs.js compatibility job. Introduces a new executable target QsSwiftComparison with a Swift harness, Node test assets, and a Bash comparator. Updates SwiftLint to exclude Tools and augments package manifests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as GitHub Actions (macos-15)
participant Swift as QsSwiftComparison (Swift)
participant Node as qs.js (Node 20)
participant Bash as compare_outputs.sh
CI->>Bash: Run ensure-compatibility job
Bash->>Node: Execute js/qs.js with test_cases.json
Node-->>Bash: Raw Encoded/Decoded stream
Bash->>Swift: Execute QsSwiftComparison
Swift-->>Bash: Raw Encoded/Decoded stream
Bash->>Bash: Normalize (sort query parts, canonicalize JSON)
alt Outputs identical
Bash-->>CI: Exit 0 with "outputs are identical"
else Outputs differ
Bash-->>CI: Exit 1 with unified diff
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
=======================================
Coverage 86.27% 86.27%
=======================================
Files 53 53
Lines 3453 3453
=======================================
Hits 2979 2979
Misses 474 474
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
.swiftlint.yml (1)
12-12: Excluding Tools entirely may hide real issues in the new Swift executableExcluding the whole
Toolstree will skip linting forQsSwiftComparison’s Swift sources. If these are developer-maintained (not generated), consider linting them (even with a lighter rule set) to keep quality consistent.Options:
- Narrow the exclusion to JS-only paths:
Tools/**/js/**.- Or re-include the Swift subfolder with an explicit include rule.
Example adjustment:
excluded: - .build - .swiftpm - .history - DerivedData - Bench - ObjCE2ETests - Tests - - Tools + - Tools/**/js/**Tools/QsSwiftComparison/js/package.json (1)
1-10: Enhance package.json for reproducibility and CI parityVerified via repository inspection:
- package-lock.json is tracked at Tools/QsSwiftComparison/js/package-lock.json and not ignored.
- CI workflow (.github/workflows/test.yml:276) pins Node to version 20.
- The helper script qs.js exists under Tools/QsSwiftComparison/js.
Proposed package.json updates:
{ "name": "qs_comparison", "version": "1.0.0", + "private": true, "description": "A comparison of query string parsing libraries", "author": "Klemen Tusar", "license": "BSD-3-Clause", + "engines": { "node": ">=20" }, + "scripts": { + "encode": "node qs.js" + }, "dependencies": { - "qs": "^6.14.0" + "qs": "6.14.0" } + // Optionally: "packageManager": "npm@9" to document your npm version }
- Pinning
qsto"6.14.0"ensures exact installs; since your lockfile is committed, you can also retain^6.14.0if you prefer relying onpackage-lock.json.private: trueprevents accidental publishing.engines.nodealigns installs with CI’s Node 20.scripts.encodestandardizes local runs ofqs.js.Tools/QsSwiftComparison/js/test_cases.json (2)
195-197: Same issue repeats across large fixtures; prefer auto-generating expected encodingsGiven the breadth of fixtures, hand-editing is error-prone. Recommend a generator to materialize
encodedfromdatausing your chosenqsoptions, checked into the repo to make diffs explicit.Example generator (adds/overwrites
encoded):// Tools/QsSwiftComparison/js/generate_expected.js const fs = require('node:fs'); const path = require('node:path'); const qs = require('qs'); const file = path.join(__dirname, 'test_cases.json'); const cases = JSON.parse(fs.readFileSync(file, 'utf8')); const options = { encodeValuesOnly: false }; // pick your policy (e.g., { format: 'RFC1738' }) for (const tc of cases) { tc.encoded = qs.stringify(tc.data, options); } fs.writeFileSync(file, JSON.stringify(cases, null, 4) + '\n'); console.log('Updated encoded expectations using qs', options);Then add a script in
package.json:"scripts": { "generate": "node generate_expected.js" }
1-20: Add more edge cases: nulls/booleans/numbers/empty arrays/undefined handlingTo increase parity coverage:
nullvalues (qs typically serializes as empty value or omits depending on options)undefined(usually omitted)- booleans and numbers
- empty arrays and empty objects
- keys containing reserved characters:
&,=,+,%,[,]- arrays with holes:
[ , "a", , "b"]- duplicate keys / order sensitivity
I can add a minimal set if you confirm desired qs options.
Tools/QsSwiftComparison/js/qs.js (2)
7-11: Harden JSON loading and make encoding explicit (utf8).Avoid relying on default encoding and add basic error handling so CI failures are clearer.
-const e2eTestCases = JSON.parse(fs.readFileSync(filePath).toString()); +let e2eTestCases; +try { + e2eTestCases = JSON.parse(fs.readFileSync(filePath, 'utf8')); +} catch (err) { + console.error('Failed to load test_cases.json from', filePath, err); + process.exit(1); +}
9-11: Lock down qs semantics explicitly; verify bracket-encoding parity with Swift.Make stringify/parse options explicit to avoid silent behavior changes across qs releases and to document expected semantics. Confirm that these match Swift’s percent-encoding (notably brackets).
-e2eTestCases.forEach(testCase => { - console.log('Encoded:', qs.stringify(testCase.data)); - console.log('Decoded:', JSON.stringify(qs.parse(testCase.encoded))); -}); +const stringifyOpts = { + encode: true, + encodeValuesOnly: false, // Keys (including brackets) will be percent-encoded. + indices: true, // a[0]=1&a[1]=2 + allowDots: false, + delimiter: '&' +}; +const parseOpts = { + ignoreQueryPrefix: false, + comma: false +}; +e2eTestCases.forEach(testCase => { + console.log('Encoded:', qs.stringify(testCase.data, stringifyOpts)); + console.log('Decoded:', JSON.stringify(qs.parse(testCase.encoded, parseOpts))); +});Follow-up:
- If Swift intends unencoded brackets, set encodeValuesOnly: true here instead (and adjust Swift) to keep parity.
Package.swift (1)
51-55: Optional: bundle test_cases.json as a resource to decouple from CWD.If you want the executable to work from arbitrary working directories (outside Tools/QsSwiftComparison), consider copying the JSON as a package resource and loading it from Bundle in main.swift.
.executableTarget( name: "QsSwiftComparison", dependencies: ["QsSwift"], - path: "Tools/QsSwiftComparison" + path: "Tools/QsSwiftComparison", + resources: [ + .copy("js/test_cases.json") + ] ),Note: main.swift would need to load from Bundle.module.url(forResource: "test_cases", withExtension: "json", subdirectory: "js").
.github/workflows/test.yml (3)
268-281: Log Node/npm versions and installed qs version for traceability.This greatly simplifies debugging when parity drifts due to upstream changes.
- name: Install Node.js 20 uses: actions/setup-node@v4 with: node-version: '20' + - name: Show Node and npm versions + run: | + node -v + npm -v + npm config get fund + npm config get audit + - name: Install qs.js working-directory: Tools/QsSwiftComparison/js - run: npm install + run: npm install + + - name: Show installed qs version + working-directory: Tools/QsSwiftComparison/js + run: npx --yes npm-which qs >/dev/null 2>&1 || true; npm ls qs || true
273-281: Prefer deterministic installs (npm ci + lockfile) to avoid flaky comparisons.Using npm install with a caret range and an ignored lockfile makes results time-dependent. Commit package-lock.json and switch to npm ci. Alternatively, pin qs to an exact version (no caret) if you don’t want a lockfile.
- - name: Install qs.js - working-directory: Tools/QsSwiftComparison/js - run: npm install + - name: Install qs.js (deterministic) + working-directory: Tools/QsSwiftComparison/js + run: npm ci --no-audit --fund=falseIf keeping npm install, at least pin the version (e.g., "qs": "6.14.0") in package.json.
282-289: Capture comparison diff as an artifact for easy inspection.When the step continues on error, surfacing the diff as an artifact helps reviewers quickly see mismatches.
- name: Compare Swift vs qs.js outputs (deterministic) if: ${{ always() }} continue-on-error: true working-directory: Tools/QsSwiftComparison env: SWIFT_DETERMINISTIC_HASHING: 1 LC_ALL: C - run: bash compare_outputs.sh + run: | + set -euo pipefail + bash compare_outputs.sh | tee compatibility.log || true + + - name: Upload compatibility diff (if any) + if: ${{ always() }} + uses: actions/upload-artifact@v4 + with: + name: qs-compatibility-diff + path: Tools/QsSwiftComparison/compatibility.log + if-no-files-found: ignoreTools/QsSwiftComparison/compare_outputs.sh (2)
3-5: Set LC_ALL in-script for hermetic sorting.LC_ALL is already set in the workflow env; exporting here makes the script hermetic when run locally.
set -euo pipefail export SWIFT_DETERMINISTIC_HASHING=1 +export LC_ALL=C
30-56: Reduce process spawn overhead: canonicalize JSON using a single Node process.canonicalize_json_with_node is invoked per Decoded line, spawning Node each time. Not a blocker, but if test cases grow this becomes noticeable. Consider a single Node helper that reads the whole stream and emits canonicalized lines, or reuse a persistent node -e script.
Happy to provide a refactor that pipes normalize_stream through a single Node process that only transforms Decoded: lines and passes others through unchanged.
Tools/QsSwiftComparison/main.swift (8)
7-13: Allow overriding test_cases.json via env var and add Bundle path fallback.This eases running from different working directories and in IDEs.
Apply:
-// Find test_cases.json (try a few common spots so you can run from repo root) +// Find test_cases.json (try a few common spots so you can run from repo root) let fm = FileManager.default let cwd = URL(fileURLWithPath: fm.currentDirectoryPath) -let jsonCandidates = [ +let envOverride = ProcessInfo.processInfo.environment["QS_TEST_CASES_JSON"].map(URL(fileURLWithPath:)) +let bundleURL = Bundle.main.resourceURL?.appendingPathComponent("js/test_cases.json") +let jsonCandidates = [ + envOverride, cwd.appendingPathComponent("Tools/QsSwiftComparison/js/test_cases.json"), cwd.appendingPathComponent("js/test_cases.json"), -] + bundleURL +].compactMap { $0 }
20-24: Handle IO/JSON errors explicitly instead of crashing later.Wrapping both read and parse in do/catch gives a cleaner failure path and exit code.
Apply:
-let data = try Data(contentsOf: jsonURL) -guard let raw = try JSONSerialization.jsonObject(with: data) as? [[String: Any]] else { - fputs("Could not parse test_cases.json as array of objects.\n", stderr) - exit(1) -} +let raw: [[String: Any]] +do { + let data = try Data(contentsOf: jsonURL) + guard let parsed = try JSONSerialization.jsonObject(with: data) as? [[String: Any]] else { + fputs("Could not parse test_cases.json as array of objects.\n", stderr) + exit(1) + } + raw = parsed +} catch { + fputs("Failed to read/parse \(jsonURL.path): \(error)\n", stderr) + exit(1) +}
32-54: Avoid Any? in normalized structures; return concrete non-optional shapes.normalizeJSON never returns nil. Returning Any? propagates [Any?] and [String: Any?] unnecessarily, which can complicate downstream encoding. Prefer [Any] and [String: Any].
Apply:
-// Recursively convert JSONSerialization output into the shapes Qs expects, -// while turning *numbers* into *strings*, like your C# helper does. -func normalizeJSON(_ v: Any) -> Any? { +// Recursively convert JSONSerialization output into the shapes Qs expects, +// while turning *numbers* into *strings*, like the other harnesses do. +func normalizeJSON(_ v: Any) -> Any { switch v { case is NSNull: return NSNull() case let n as NSNumber: return isBoolNSNumber(n) ? n.boolValue : n.stringValue case let s as String: return s case let a as [Any]: - return a.map(normalizeJSON) + return a.map(normalizeJSON) case let d as [String: Any]: - var out: [String: Any?] = [:] + var out: [String: Any] = [:] out.reserveCapacity(d.count) for (k, vv) in d { out[k] = normalizeJSON(vv) } return out default: return String(describing: v) } }Outside this hunk, adjust the call sites to drop optionals:
// Line 173: -let dataIn = c["data"].flatMap(normalizeJSON) +let dataIn = c["data"].map(normalizeJSON) // Line 177: -let s = try! Qs.encode((dataIn as? [String: Any?]) ?? [:]) +let s = try! Qs.encode((dataIn as? [String: Any]) ?? [:])
56-78: Escape U+2028 and U+2029 for cross-runtime JSON compatibility.JSON.stringify in JS escapes these separators; doing the same avoids spurious diffs.
Apply:
case 0x09: out += "\\t" +case 0x2028: out += "\\u2028" // line separator +case 0x2029: out += "\\u2029" // paragraph separator case 0x00..<0x20:
93-103: Normalize exponent formatting to match JSON.stringify (strip zero padding).String(format: "%g") emits exponents like e-07; JSON.stringify uses e-7. Normalize to reduce false diffs.
Apply:
- // Use a locale-independent representation - out += String(format: "%g", n.doubleValue) + // Use a locale-independent representation and normalize exponent zero padding + var s = String(format: "%g", n.doubleValue).lowercased() + s = s.replacingOccurrences(of: "e+0", with: "e+") + s = s.replacingOccurrences(of: "e-0", with: "e-") + out += s
170-181: Prefer graceful error reporting over try! to keep processing all cases.A single failing case will crash the process and produce partial output, making comparison noisy. Recommend per-case do/catch that emits a clear error line and continues.
Apply:
- let encodedOut: String = { - let s = try! Qs.encode((dataIn as? [String: Any?]) ?? [:]) - return percentEncodeBrackets - ? s.replacingOccurrences(of: "[", with: "%5B").replacingOccurrences(of: "]", with: "%5D") : s - }() + let encodedOut: String = { + do { + let s = try Qs.encode((dataIn as? [String: Any]) ?? [:]) + return percentEncodeBrackets + ? s.replacingOccurrences(of: "[", with: "%5B").replacingOccurrences(of: "]", with: "%5D") + : s + } catch { + return "ERROR:\(String(describing: error))" + } + }()Follow-up: If you adopt this, mirror the “error-as-string” contract in the JS harness for fair comparison.
184-193: Mirror graceful error handling on decode and consider labeling cases.Use do/catch here as well; optionally print a case index or name to simplify triage in diffs.
Apply:
- if let encoded = encodedIn { - let decoded = try! Qs.decode(encoded) - print("Decoded: \(canonJSON(decoded))") - } else { - // If a case doesn’t have 'encoded', decode what we just encoded (parity) - let decoded = try! Qs.decode(encodedOut) - print("Decoded: \(canonJSON(decoded))") - } + if let encoded = encodedIn { + do { + let decoded = try Qs.decode(encoded) + print("Decoded: \(canonJSON(decoded))") + } catch { + print("Decoded: ERROR:\(String(describing: error))") + } + } else { + // If a case doesn’t have 'encoded', decode what we just encoded (parity) + do { + let decoded = try Qs.decode(encodedOut) + print("Decoded: \(canonJSON(decoded))") + } catch { + print("Decoded: ERROR:\(String(describing: error))") + } + }Optional prefix per case for easier diffing:
// Before printing, add e.g.: if let name = c["name"] as? String { print("Case: \(name)") }
5-5: Make bracket-encoding behavior configurable and sync with JS harnessThe Swift harness still hard-codes
percentEncodeBracketsand its comment only mentions C#. The JS harness likewise has no equivalent toggle. To allow CI/local overrides and keep all three harnesses (Swift, JS, C#) in sync, introduce an environment-driven flag and update the inline comments accordingly.• Tools/QsSwiftComparison/main.swift (line 5)
Replace the hard-coded toggle and C#-only comment with an env-aware switch and reference both JS/C# harnesses.-// Keep this in sync with your C# harness toggle -let percentEncodeBrackets = true +// Keep this in sync with your JS/C# harness toggle. Override via QS_ENCODE_BRACKETS=1|true|yes|on. +let percentEncodeBrackets: Bool = { + if let v = ProcessInfo.processInfo.environment["QS_ENCODE_BRACKETS"]?.lowercased() { + return ["1", "true", "yes", "on"].contains(v) + } + return true +}()• Tools/QsSwiftComparison/js/qs.js
Introduce a similar check aroundqs.stringify, e.g.:const encodeBrackets = (() => { const v = (process.env.QS_ENCODE_BRACKETS || '').toLowerCase(); return ['1','true','yes','on'].includes(v) ? true : true; // default true })(); e2eTestCases.forEach(testCase => { console.log('Encoded:', qs.stringify(testCase.data, { encode: encodeBrackets })); console.log('Decoded:', JSON.stringify(qs.parse(testCase.encoded))); });This ensures both harnesses respect the same
QS_ENCODE_BRACKETSvariable and removes drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks).swiftlint.yml(1 hunks).vscode/launch.json(1 hunks)Package.swift(1 hunks)Package@swift-5.10.swift(1 hunks)Tools/QsSwiftComparison/compare_outputs.sh(1 hunks)Tools/QsSwiftComparison/js/.gitignore(1 hunks)Tools/QsSwiftComparison/js/package.json(1 hunks)Tools/QsSwiftComparison/js/qs.js(1 hunks)Tools/QsSwiftComparison/js/test_cases.json(1 hunks)Tools/QsSwiftComparison/main.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Package.swift (1)
Tools/QsSwiftComparison/js/qs.js (1)
path(2-2)
Package@swift-5.10.swift (1)
Tools/QsSwiftComparison/js/qs.js (1)
path(2-2)
Tools/QsSwiftComparison/compare_outputs.sh (1)
Tests/QsObjCTests/ObjCFilterTests.swift (1)
parts(11-14)
⏰ 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). (4)
- GitHub Check: Objective-C E2E tests (Xcode) (macos-15, 16.3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (swift)
🔇 Additional comments (7)
Tools/QsSwiftComparison/js/package.json (1)
1-10: To verify there are no open advisories on 6.14.x and to see if a newer compatible version exists, could you run these commands in your local environment and paste the outputs? Then I can interpret the results and advise whether to pin at 6.14.0 or bump to a more recent release.• Check the latest published version:
npm view qs version• See recent version history (last 20 versions):
npm view qs versions --json | tail -n 20• Audit for any qs advisories in your project’s deps:
npm audit --json | jq '.advisories | to_entries[] | .value | select(.module_name=="qs")'• (Optional) Run a Snyk test on qs@6.14.0:
npx snyk test qs@6.14.0• (Optional) Inspect the npm registry metadata directly:
curl -s https://registry.npmjs.org/qs | jq '.time, .versions | keys[-5:]'With those outputs, I can confirm whether 6.14.x is free of advisories and if upgrading is warranted.
Tools/QsSwiftComparison/js/test_cases.json (1)
149-156: Confirm Swift-side space encoding behavior
I couldn’t find any in-repo implementation ofQs.encode(main.swift simply calls into the external Qs Swift module), so I can’t tell whether it percent-encodes spaces as%20(RFC3986) or as+(RFC1738). Please verify which format the Swift encoder uses and then:
- Update the JSON fixture’s
encodedvalues to match that behavior (e.g. use%20or+for spaces), or- Pass the matching
{ format: 'RFC1738' }(or a custom encoder) into your JavaScript harness so both sides agree, and document the choice.Package.swift (1)
51-55: LGTM: Executable target registered correctly.The QsSwiftComparison target is scoped to Tools and depends only on QsSwift. Looks good.
Package@swift-5.10.swift (1)
64-68: LGTM: Manifest parity for Swift 5.10.The executable target mirrors the primary manifest. Consistent and correct.
Tools/QsSwiftComparison/main.swift (3)
15-18: Good failure message for missing fixtures.Clear, actionable error with all attempted paths. LGTM.
26-31: Robust NSNumber–Bool detection.Using CFGetTypeID against CFBoolean is the right call here for correctness across bridges.
130-157: Deterministic key ordering incl. NSDictionary path looks solid.Converting non-String keys using String(describing:) and re-sorting ensures stable output; good for diffability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/test.yml (7)
252-256: Ensure the compatibility job still runs when earlier jobs fail.Right now, if analyze/tests fail, this job won’t run due to the needs dependency. If you want a “best-effort” signal regardless of prior failures, add an explicit always condition at the job level.
ensure-compatibility: name: Ensure compatibility with qs.js needs: [ analyze, tests ] + if: ${{ always() }} runs-on: macos-15 timeout-minutes: 15
274-277: Enable npm cache for faster installs.Leverage setup-node’s built-in caching pointed at the subdir lockfile.
- name: Install Node.js uses: actions/setup-node@v4 with: - node-version: '20' + node-version: '20' + cache: 'npm' + cache-dependency-path: Tools/QsSwiftComparison/js/package-lock.json
274-277: Nit: consider pinning to 20.x for automatic patch updates.Using 20.x keeps you on the latest Node 20 patch without manual bumps, while staying on the same major. If you prefer fully deterministic runners, keep '20'.
- node-version: '20' + node-version: '20.x'
279-281: Use npm ci for reproducible installs in CI.npm ci is faster and respects the lockfile; also skip audit/fund noise.
- name: Install qs.js working-directory: Tools/QsSwiftComparison/js - run: npm install + run: npm ci --no-audit --no-fund
282-286: Also print Node and npm versions for traceability.Helps correlate runs when debugging.
- name: Show qs.js version working-directory: Tools/QsSwiftComparison/js run: | + node --version + npm --version echo "qs version: $(node -p "require('qs/package.json').version")"
287-293: Cache SwiftPM artifacts to speed the Swift side of the harness.Mirrors your tests job and reduces repeated dependency resolution for the Swift executable.
- name: Show qs.js version working-directory: Tools/QsSwiftComparison/js run: | echo "qs version: $(node -p "require('qs/package.json').version")" + - name: Cache SwiftPM artifacts (QsSwiftComparison) + uses: actions/cache@v4 + with: + path: | + ~/.swiftpm + .build + ~/Library/Caches/org.swift.swiftpm + key: spm-${{ runner.os }}-xcode-16.3-${{ hashFiles('**/Package.resolved') }} + restore-keys: | + spm-${{ runner.os }}-xcode-16.3- + - name: Compare Swift vs qs.js outputs (deterministic) continue-on-error: true working-directory: Tools/QsSwiftComparison env: SWIFT_DETERMINISTIC_HASHING: 1 LC_ALL: C run: bash compare_outputs.sh
287-294: Preserve diffs and outputs when mismatches occur.Upload artifacts so developers can inspect discrepancies offline, even though the step is non-blocking.
- name: Compare Swift vs qs.js outputs (deterministic) continue-on-error: true working-directory: Tools/QsSwiftComparison env: SWIFT_DETERMINISTIC_HASHING: 1 LC_ALL: C run: bash compare_outputs.sh + + - name: Upload comparison artifacts + if: ${{ always() }} + uses: actions/upload-artifact@v4 + with: + name: qs-compatibility-artifacts + path: | + Tools/QsSwiftComparison/**/*.{diff,out,txt,log} + if-no-files-found: ignore
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test.yml(1 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). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (swift)
- GitHub Check: Codacy Security Scan
🔇 Additional comments (1)
.github/workflows/test.yml (1)
252-294: Good addition: non-blocking qs.js compatibility check is integrated cleanly.Job is clearly scoped, uses deterministic env (SWIFT_DETERMINISTIC_HASHING, LC_ALL), and prints the qs version for traceability.
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)
.github/workflows/test.yml (1)
79-89: Include Xcode in SPM cache key to avoid ABI-incompatible restoresThe tests job’s cache key only includes SWIFT_VERSION. Binary artifacts can become incompatible across Xcode point releases; include XCODE_VERSION to prevent subtle build/test flakiness when the Xcode image changes.
Apply this diff:
- key: spm-${{ runner.os }}-swift-${{ env.SWIFT_VERSION }}-${{ hashFiles('**/Package.resolved') }} + key: spm-${{ runner.os }}-xcode-${{ env.XCODE_VERSION }}-swift-${{ env.SWIFT_VERSION }}-${{ hashFiles('**/Package.resolved') }} restore-keys: | - spm-${{ runner.os }}-swift-${{ env.SWIFT_VERSION }}- + spm-${{ runner.os }}-xcode-${{ env.XCODE_VERSION }}-swift-${{ env.SWIFT_VERSION }}-
🧹 Nitpick comments (4)
.github/workflows/test.yml (4)
108-114: Add Xcode to Codecov flags/name for consistency with ObjC jobMinor but helpful for filtering/analytics symmetry: the ObjC upload flags include xcode-${{ env.XCODE_VERSION }}, while Swift’s do not. Consider aligning.
Apply this diff:
- flags: swift,macos-15,swift-${{ env.SWIFT_VERSION }} - name: QsSwift-macos-15-swift-${{ env.SWIFT_VERSION }} + flags: swift,macos-15,xcode-${{ env.XCODE_VERSION }},swift-${{ env.SWIFT_VERSION }} + name: QsSwift-macos-15-xcode-${{ env.XCODE_VERSION }}-swift-${{ env.SWIFT_VERSION }}
141-165: Symlink workaround: confirm ongoing necessity on macOS-15 imagesThis step writes a sibling symlink to the parent of GITHUB_WORKSPACE. It’s fine on hosted runners, but it’s brittle if the workflow is re-used via workflow_call in different environments.
If it’s no longer needed (e.g., recent SPM/Xcode pathing is stable), consider removing it. Otherwise, gate it behind a check to skip when the parent isn’t writable, without failing the job.
- test -w "$PARENT" + if ! test -w "$PARENT"; then + echo "Parent $PARENT not writable; skipping symlink workaround." + exit 0 + fi
264-274: Align ensure-compatibility SPM cache key schema with tests jobHere you key by Xcode only, while the tests job keys by Swift (after the change above: both). Use the same scheme to maximize cache hits and avoid unnecessary duplication.
Apply this diff:
- key: spm-${{ runner.os }}-xcode-${{ env.XCODE_VERSION }}-${{ hashFiles('**/Package.resolved') }} + key: spm-${{ runner.os }}-xcode-${{ env.XCODE_VERSION }}-swift-${{ env.SWIFT_VERSION }}-${{ hashFiles('**/Package.resolved') }} restore-keys: | - spm-${{ runner.os }}-xcode-${{ env.XCODE_VERSION }}- + spm-${{ runner.os }}-xcode-${{ env.XCODE_VERSION }}-swift-${{ env.SWIFT_VERSION }}-
253-262: Optional: assert toolchain drift earlyYou print versions but don’t check them. If SWIFT_VERSION and the selected toolchain drift, caches/labels become misleading. Consider a non-fatal guard that warns on mismatch.
Add after “Show Xcode & Swift versions”:
+ - name: Warn if Swift toolchain doesn’t match env.SWIFT_VERSION + run: | + set -euo pipefail + swift --version | tee swift-version.txt + if ! swift --version | grep -q "${{ env.SWIFT_VERSION }}"; then + echo "::warning::Swift toolchain version differs from SWIFT_VERSION=${{ env.SWIFT_VERSION }}." + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Tools/QsSwiftComparison/js/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
.github/workflows/test.yml(10 hunks)Tools/QsSwiftComparison/js/.gitignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Tools/QsSwiftComparison/js/.gitignore
⏰ 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). (3)
- GitHub Check: Swift tests + coverage
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (swift)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
17-20: Good centralization of toolchain versionsDefining XCODE_VERSION and SWIFT_VERSION at the workflow root simplifies maintenance and keeps jobs consistent. LGTM.
166-176: Good cache scoping for Xcode SPM directoriesIncluding both ~/Library and DerivedData SPM paths is thorough and helps ObjC job performance. LGTM.
242-247: New qs.js compatibility job: solid structureRunning after analyze/tests, pinning Node 20, and using deterministic env (SWIFT_DETERMINISTIC_HASHING, LC_ALL=C) are all good choices for reproducibility. Nice addition.
…e keys, enhance symlink workaround, and refine Codecov flags
…, display Node and npm versions
This pull request adds a new cross-language comparison tool for query string encoding/decoding between Swift and JavaScript (using the
qslibrary). It introduces a new Swift executable target, a Node.js test harness, and a GitHub Actions workflow to automate compatibility checks. These changes will help ensure that Swift’s query string handling matches established JavaScript semantics for a wide range of test cases.Compatibility tooling and workflow:
QsSwiftComparisonto the package, located atTools/QsSwiftComparison, which is used for output comparison against JavaScript’sqslibrary. (Package.swift,Package@swift-5.10.swift) [1] [2]ensure-compatibility) that installs Node.js, runs the comparison tool, and checks output consistency between Swift and JavaScript implementations. (.github/workflows/test.yml)JavaScript test harness and cases:
js/qs.js) and a comprehensive set of query string encoding/decoding test cases (js/test_cases.json) to validate cross-language compatibility. (Tools/QsSwiftComparison/js/qs.js,Tools/QsSwiftComparison/js/test_cases.json) [1] [2]package.jsonto manage theqsdependency and a.gitignorefor Node modules and lock files. (Tools/QsSwiftComparison/js/package.json,Tools/QsSwiftComparison/js/.gitignore) [1] [2]Comparison logic and normalization:
compare_outputs.sh) that normalizes outputs from both Swift and JavaScript tools, sorting keys and query parameters to avoid spurious diffs due to ordering, and produces unified diff output for discrepancies. (Tools/QsSwiftComparison/compare_outputs.sh)Development environment:
.vscode/launch.json)Toolsdirectory from linting. (.swiftlint.yml)Summary by CodeRabbit
New Features
Tests
Chores
Style