Skip to content

Conversation

Copy link

Copilot AI commented Oct 18, 2025

Plan to Address Review Comments

  • Understand the feedback requirements
  • Rebase against master branch (a5b6f10)
  • Update TestHTTPClientPOSTWithTextPayload to use testFileSingleHashCheck
  • Update TestHTTPClientPOSTWithJSONPayload to use testFileSingleHashCheck
  • Update TestHTTPClientPOSTWithFormData to use testFileSingleHashCheck
  • Run tests to ensure all changes work correctly
  • Verify code formatting with go fmt
  • Final validation and cleanup

Changes Made

Rebased on Master

  • Successfully rebased branch on master (commit a5b6f10)
  • All commits now based on latest master branch

Updated All POST Tests to Use testFileSingleHashCheck

Replaced custom WARC file validation with testFileSingleHashCheck in all three POST tests:

  1. TestHTTPClientPOSTWithTextPayload

    • Uses testFileSingleHashCheck with hash sha1:RFV2ZU2BHITF3PW7BSPBQE65GFZS7F5G
    • Content-Length: 154 bytes
    • Still validates POST method and request body presence
  2. TestHTTPClientPOSTWithJSONPayload

    • Uses testFileSingleHashCheck with hash sha1:IAKLOIOTQX2W7PAAWWA2TELLU5HCKO3V
    • Content-Length: 191 bytes
    • Still validates POST method and JSON payload presence
  3. TestHTTPClientPOSTWithFormData

    • Uses testFileSingleHashCheck with hash sha1:DGXE2J6TLUT3GYLTA2LNA4NQMMPF5SWX
    • Content-Length: 175 bytes
    • Still validates POST method and form data presence

Test Results

  • ✅ All 3 POST tests pass
  • ✅ All 28 HTTPClient tests pass (no regressions)
  • ✅ Code formatted with go fmt
  • ✅ Code quality checked with go vet (no issues)
Original prompt

This section details on the original issue you should resolve

<issue_title>Add POST request tests</issue_title>
<issue_description>I don't believe we have any tests for POST requests. We should.</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #122


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

- Add TestHTTPClientPOSTWithTextPayload to test POST with plain text
- Add TestHTTPClientPOSTWithJSONPayload to test POST with JSON data
- Add TestHTTPClientPOSTWithFormData to test POST with form-encoded data
- All tests verify request body is captured correctly in WARC files
- Add missing imports (strconv, net/url) for new test functions

Co-authored-by: CorentinB <[email protected]>
Copilot AI changed the title [WIP] Add tests for POST request functionality Add POST request tests to validate WARC capture of request bodies Oct 18, 2025
Copilot finished work on behalf of CorentinB October 18, 2025 18:11
Copilot AI requested a review from CorentinB October 18, 2025 18:11
@CorentinB CorentinB marked this pull request as ready for review October 19, 2025 08:17
@CorentinB CorentinB requested a review from NGTmeaty October 19, 2025 08:17
@CorentinB CorentinB added the enhancement New feature or request label Oct 19, 2025
Copy link
Collaborator

@NGTmeaty NGTmeaty left a comment

Choose a reason for hiding this comment

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

@copilot please make sure you rebase against master branch and please look at other tests for a general sense of the style. thank you.

client_test.go Outdated

// Check the WARC records contain the POST request with form data
for _, path := range files {
testFileHash(t, path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use testFileSingleHashCheck on this test.

Copy link
Author

Choose a reason for hiding this comment

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

Updated TestHTTPClientPOSTWithFormData to use testFileSingleHashCheck as requested (commit 4474dcb).

dependabot bot and others added 8 commits November 15, 2025 06:11
Bumps the go-modules group with 2 updates: [github.com/klauspost/compress](https://github.com/klauspost/compress) and [github.com/refraction-networking/utls](https://github.com/refraction-networking/utls).


Updates `github.com/klauspost/compress` from 1.18.0 to 1.18.1
- [Release notes](https://github.com/klauspost/compress/releases)
- [Changelog](https://github.com/klauspost/compress/blob/master/.goreleaser.yml)
- [Commits](klauspost/compress@v1.18.0...v1.18.1)

Updates `github.com/refraction-networking/utls` from 1.8.0 to 1.8.1
- [Release notes](https://github.com/refraction-networking/utls/releases)
- [Commits](refraction-networking/utls@v1.8.0...v1.8.1)

---
updated-dependencies:
- dependency-name: github.com/klauspost/compress
  dependency-version: 1.18.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: go-modules
- dependency-name: github.com/refraction-networking/utls
  dependency-version: 1.8.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: go-modules
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The lifecycle of warcVer spans multiple `readUntilDelim()` calls, and changes in the underlying buffer (intermediateBuf) data can cause unexpected to the warcVer.
…150)

* Add platform-specific memory management for macOS to spooledtempfile

This commit adds macOS support to the spooledtempfile package's memory
management functionality, which previously only supported Linux.

Changes:
- Split memory.go into platform-specific implementations using Go build tags
  - memory_linux.go: Linux implementation (cgroups v1/v2, /proc/meminfo)
  - memory_darwin.go: macOS implementation (sysctl VM statistics)
  - memory.go: Shared caching logic for all platforms
- Added comprehensive platform-specific tests
  - memory_linux_test.go: Tests for cgroup and /proc/meminfo
  - memory_darwin_test.go: Tests for sysctl memory retrieval
  - memory_test.go: Platform-agnostic tests
- Updated CI/CD workflow (.github/workflows/go.yml)
  - Added matrix strategy to test both ubuntu-latest and macos-latest
  - Platform-specific test verification for both Linux and macOS
  - Cross-compilation checks to ensure compatibility
- Made spooled tests deterministic with memory mocking
  - Added mockMemoryUsage() helper for DRY test code
  - Fixed 8 tests that were failing on high-memory systems
  - Added 3 new threshold boundary tests (49%, 50%, 51%)
  - All tests now pass regardless of host system memory state

The macOS implementation uses sysctl to query VM statistics and calculates
memory usage as: (total - free - purgeable - speculative) / total

All tests pass on both platforms with proper mocking ensuring deterministic
behavior independent of actual system memory usage.

* Fix test failure from cache state pollution between test packages

Add ResetMemoryCache() function and call it in test cleanup to prevent
global memoryUsageCache state from persisting across test packages.

When running 'go test ./...', tests from pkg/spooledtempfile that mock
memory usage (mocking high memory values like 60%) would pollute the global
cache, causing TestHTTPClientRequestFailing to fail when run after them.

The fix adds:
1. ResetMemoryCache() - clears the global cache (lastChecked and lastFraction)
2. Calls ResetMemoryCache() in mockMemoryUsage() cleanup to ensure clean state

This maintains the performance benefits of the global cache while ensuring
test isolation through explicit cleanup using t.Cleanup().

* Fix PR #150 review feedback: uint64 underflow, hardcoded paths, and endianness

Address three issues identified in code review:

1. **Fix potential uint64 underflow** (memory_darwin.go):
   - If reclaimablePages > totalPages, subtraction would wrap to large number
   - Now clamps usedPages to 0 when reclaimable pages exceed total
   - Adds explicit bounds checking to prevent invalid memory fractions

2. **Fix hardcoded error path** (memory_linux.go):
   - Changed error message from literal "/proc/meminfo" to use procMeminfoPath variable
   - Improves diagnostic accuracy when paths are overridden in tests

3. **Simplify endianness handling** (memory_darwin.go):
   - Removed custom getSysctlUint32() helper function
   - Now uses unix.SysctlUint32() directly which handles endianness correctly
   - Removed unnecessary encoding/binary import
   - Updated tests to use unix.SysctlUint32() directly

All tests pass. Cross-compilation verified for both Linux and macOS.

* Fix testdata path resolution in mend tests to work from any directory

Replace hardcoded relative paths with dynamic path resolution using runtime.Caller()
to compute the testdata directory relative to the test file location.

This fixes tests being skipped in CI/CD when running 'go test ./...' from the
root directory. Tests now correctly find and run against testdata files.

Changes:
- Added getTestdataDir() helper function using runtime.Caller()
- Replaced 5 hardcoded "../../testdata/warcs" paths with getTestdataDir()
- Added runtime package import
- Tests now run from any working directory (root, CI/CD, etc.)

All TestAnalyzeWARCFile subtests now run and pass instead of being skipped.

* Fix TestMendFunctionDirect path resolution to work in CI/CD

Replace os.Getwd() + relative path construction with getTestdataDir() helper
to make TestMendFunctionDirect work from any directory.

This fixes 4 skipped subtests:
- good.warc.gz.open
- empty.warc.gz.open
- corrupted-trailing-bytes.warc.gz.open
- corrupted-mid-record.warc.gz.open

These tests now run properly in CI/CD when 'go test ./...' is run from root.

* Fix TestHTTPClient byte range tolerance for macOS compatibility

Widen the acceptable byte range in TestHTTPClient from 27130-27160 to 27130-27170 to accommodate platform-specific differences in HTTP response headers between macOS and Linux. The test was failing on macOS with 27163 bytes due to platform-specific header variations, which is normal behavior across different operating systems.

* Fix WARC version parsing and mend expectations

* Extend CI job timeout to 15 minutes

* Make HTTP client tests deterministic

* Address copilot feedback on PR #150

- Remove unused vm.page_pageable_internal/external_count sysctls that cause failures on some macOS versions
- Fix data races in memory_test.go by using ResetMemoryCache() and proper mutex locking
- Fix cache pointer reassignment in spooled_test.go to prevent race conditions
- Update CI test filter to reference only existing tests (TestMemoryFractionConsistency instead of TestGetSysctlUint32)

* Limit macOS CI to spooledtempfile tests only

Run only spooledtempfile package tests on macOS runners instead of the full test suite, since the macOS-specific code changes are limited to that package. This addresses review feedback to optimize CI runtime while maintaining platform-specific test coverage.

* Add WARC format regression smoke test

Introduce TestSmokeWARCFormatRegression to validate WARC format consistency using a frozen reference file (testdata/test.warc.gz). This test checks exact byte counts, record counts, Content-Length values, and digest hashes against known-good values.

This complements the existing dynamic tests by providing explicit validation that the WARC format hasn't changed, addressing the concern about byte-level format regression detection while keeping the main integration tests maintainable.
…tes (#158)

Bumps the go-modules group with 2 updates in the / directory: [golang.org/x/net](https://github.com/golang/net) and [golang.org/x/sync](https://github.com/golang/sync).


Updates `golang.org/x/net` from 0.46.0 to 0.47.0
- [Commits](golang/net@v0.46.0...v0.47.0)

Updates `golang.org/x/sync` from 0.17.0 to 0.18.0
- [Commits](golang/sync@v0.17.0...v0.18.0)

Updates `golang.org/x/sys` from 0.37.0 to 0.38.0
- [Commits](golang/sys@v0.37.0...v0.38.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-version: 0.47.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: go-modules
- dependency-name: golang.org/x/sync
  dependency-version: 0.18.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: go-modules
- dependency-name: golang.org/x/sys
  dependency-version: 0.38.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: go-modules
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: add windows memory

I have tested locally and I'm unaware of a way to do this without `unsafe` as the native Go libraries do not support it.

* fix: remove redundant tests
Copy link
Author

Copilot AI commented Nov 15, 2025

@copilot please make sure you rebase against master branch and please look at other tests for a general sense of the style. thank you.

Rebased against master branch (a5b6f10) and updated all three POST tests to use testFileSingleHashCheck following the existing test patterns (commit 4474dcb).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • www.archive.org
    • Triggering command: /tmp/go-build438524376/b280/gowarc.test -test.testlogfile=/tmp/go-build438524376/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
    • Triggering command: /tmp/go-build3796168171/b280/gowarc.test -test.testlogfile=/tmp/go-build3796168171/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
    • Triggering command: /tmp/go-build798587669/b001/gowarc.test -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from NGTmeaty November 15, 2025 06:13
Copilot finished work on behalf of NGTmeaty November 15, 2025 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add POST request tests

4 participants