Project

General

Profile

Actions

Bug #23025

closed

Fix ContainerHTTPProxy ReusedPort integration tests to use builtin go http client instead of curl

Added by Tom Clegg 9 months ago. Updated 8 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Tests
Target version:
Story points:
-
Release relationship:
Auto

Description


Subtasks 1 (0 open1 closed)

Task #23069: Review 23025-test-with-stdlibResolvedTom Clegg08/06/2025Actions

Related issues 2 (1 open1 closed)

Related to Arvados - Idea #22859: Support service containers on a single hostResolvedTom Clegg06/25/2025Actions
Related to Arvados Epics - Idea #17207: services running in containersIn Progress03/01/202508/31/2025Actions
Actions #1

Updated by Tom Clegg 9 months ago

  • Related to Idea #22859: Support service containers on a single host added
Actions #2

Updated by Tom Clegg 9 months ago

  • Related to Idea #17207: services running in containers added
Actions #3

Updated by Brett Smith 8 months ago

  • Target version set to Development 2025-08-06
  • Assigned To set to Tom Clegg
  • Category set to Tests
Actions #4

Updated by Brett Smith 8 months ago

  • Subtask #23069 added
Actions #5

Updated by Tom Clegg 8 months ago

  • Status changed from New to In Progress
Actions #6

Updated by Tom Clegg 8 months ago

23025-test-with-stdlib @ 186c62292d1b2ea88ed8573ffb26e0a7a44d8d1b -- developer-run-tests: #4844

  • All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
    • ✅ rewrote tests to use stdlib http client instead of curl
    • ✨ fixed double-redirect that wasn't caught by curl tests
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • n/a
  • Code is tested and passing, both automated and manual, what manual testing was done is described.
    • ✅ updated tests passed
  • The tested code incorporates recent main branch changes.
  • New or changed UI/UX has gotten feedback from stakeholders.
    • n/a
  • Documentation has been updated.
    • n/a
  • Behaves appropriately at the intended scale (describe intended scale).
    • n/a
  • Considered backwards and forwards compatibility issues between client and server.
    • n/a
  • Follows our coding standards and GUI style guidelines.
Actions #7

Updated by Brett Smith 8 months ago

I was starting to feel bad about prioritizing this so highly. I guess I can at least console myself with the fact that it caught a real bug.

In the tests, the struct name testReusedPortCurl and associated comment are now both out-of-date. Can we please update both?

I assume testContainerHTTPProxyUsingCurl still uses curl because it was added in #22760, not #22859, therefore it is out of scope? I am fine with that, please do not update more tests, just wondering if I understand. If there's a specific reason these tests are written with curl than stdlib, that might be helpful to note in a comment for future understanding.

Thanks.

Actions #8

Updated by Tom Clegg 8 months ago

Brett Smith wrote in #note-7:

I was starting to feel bad about prioritizing this so highly. I guess I can at least console myself with the fact that it caught a real bug.

Same.

In the tests, the struct name testReusedPortCurl and associated comment are now both out-of-date. Can we please update both?

Updated.

I assume testContainerHTTPProxyUsingCurl still uses curl because it was added in #22760, not #22859, therefore it is out of scope? I am fine with that, please do not update more tests, just wondering if I understand. If there's a specific reason these tests are written with curl than stdlib, that might be helpful to note in a comment for future understanding.

Right, no particular reason. I've added a comment saying so.

23025-test-with-stdlib @ 8779797108ee2d1e3baa690fd22aaf54362ee76b -- developer-run-tests: #4845

Actions #9

Updated by Brett Smith 8 months ago

Tom Clegg wrote in #note-8:

23025-test-with-stdlib @ 8779797108ee2d1e3baa690fd22aaf54362ee76b -- developer-run-tests: #4845

LGTM, thanks.

Actions #10

Updated by Tom Clegg 8 months ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF