docker / docker-credential-helpers

Programs to keep Docker login credentials safe by storing in platform keystores
MIT License
1.09k stars 172 forks source link

use designated domains in tests (RFC2606) #274

Open thaJeztah opened 1 year ago

thaJeztah commented 1 year ago

Update domains used in tests to used domains that are designated for this purpose as described in RFC2606, section 3

codecov-commenter commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 50.72%. Comparing base (6b9df3e) to head (b737c5a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #274 +/- ## ========================================== - Coverage 55.28% 50.72% -4.57% ========================================== Files 9 8 -1 Lines 624 483 -141 ========================================== - Hits 345 245 -100 + Misses 234 201 -33 + Partials 45 37 -8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thaJeztah commented 1 year ago

oh! found some more; hold on a minute

hm.. also interested why CI fails; need to check

crazy-max commented 1 year ago

oh! found some more; hold on a minute

hm.. also interested why CI fails; need to check

Hum yes it seems path query is not taken into account anymore https://github.com/docker/docker-credential-helpers/actions/runs/5098464489/jobs/9165620051?pr=274#step:7:66

=== RUN   TestWinCredHelperStoreRetrieve
    wincred_windows_test.go:230: Error: expected username user-7, got username user-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar
    wincred_windows_test.go:233: Error: expected secret secret-7, got secret secret-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar
crazy-max commented 1 year ago

It should exact match properly though https://github.com/docker/docker-credential-helpers/blob/11e6d3772ce263a74a2d150e858f23b975c4e374/wincred/wincred_windows.go#L110

thaJeztah commented 1 year ago

Yeah; want to give it a look indeed.

Also change these tests into subtests probably

thaJeztah commented 1 year ago

(only addressed the "found some more" - not looked at the failures yet)

thaJeztah commented 1 year ago

I rebased this PR on top of the other PR that adds subtests;

=== RUN   TestWinCredHelperStoreRetrieve/https://foobar.example.com:2376/some/other/path?foo=bar
    wincred_windows_test.go:274: Error: expected username user-7, got username user-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar
    wincred_windows_test.go:277: Error: expected secret secret-7, got secret secret-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar

The test that's failing uses the same domain and path as the test before it, but differs in query parameters https://github.com/docker/docker-credential-helpers/blob/f09e79d7419a4122052bc8fc24f2a36aaecc2495/wincred/wincred_windows_test.go#L195-L196

And the test is using a test-table, which adds the credentials as part of the table for each test, but does not delete the credentials after testing; https://github.com/docker/docker-credential-helpers/blob/f09e79d7419a4122052bc8fc24f2a36aaecc2495/wincred/wincred_windows_test.go#L213-L235

I wonder if the issue is if

The .Add() is designed to effectively be an "Upsert" (create if not exists, otherwise update). For the macOS keychain helper, I noticed that there's an explicit Delete() at the start; https://github.com/docker/docker-credential-helpers/blob/f09e79d7419a4122052bc8fc24f2a36aaecc2495/osxkeychain/osxkeychain_darwin.go#L35-L37

The wincred.Add() inmplementation does not have that; https://github.com/docker/docker-credential-helpers/blob/f09e79d7419a4122052bc8fc24f2a36aaecc2495/wincred/wincred_windows.go#L16-L26

Some of the other tests call a Delete() as part of the test, e.g. TestWinCredHelperRetrieveAliases; https://github.com/docker/docker-credential-helpers/blob/f09e79d7419a4122052bc8fc24f2a36aaecc2495/wincred/wincred_windows_test.go#L128

So, I suspect what happens is that;