search

notaryproject / notation-core-go

Contains support for Notary Project signature envelope, and format specific implementation
Apache License 2.0
14 stars 28 forks source link

fix(test): test cases with `example.com` URL #238

Closed JeyJeyGao closed 1 week ago

JeyJeyGao commented 2 weeks ago

Fix:

NOTE: .test is a reserved domain for testing in RFC 2606

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 91.29%. Comparing base (3067ab1) to head (1c112ba). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #238 +/- ## ========================================== - Coverage 91.89% 91.29% -0.61% ========================================== Files 30 30 Lines 2074 2608 +534 ========================================== + Hits 1906 2381 +475 - Misses 114 173 +59 Partials 54 54 ```

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

JeyJeyGao commented 2 weeks ago

I would suggest replacing any real URLs in test files with fake ones even though we are mocking the servers. This is to avoid future mistaken tests that a developer forgot to mock the server and a real roundtrip got sent out.

How can we ensure the URL is fake? Maybe the fake one will be registered by someone else later.

JeyJeyGao commented 2 weeks ago

I would suggest replacing any real URLs in test files with fake ones even though we are mocking the servers. This is to avoid future mistaken tests that a developer forgot to mock the server and a real roundtrip got sent out.

Replaced example.com with example.fake

Two-Hearts commented 2 weeks ago

How can we ensure the URL is fake? Maybe the fake one will be registered by someone else later.

Oh, I was thinking we are mocking the server using https://pkg.go.dev/net/http/httptest#Server. Then we could use the server.URL directly, turns out we are mocking the http client transport.

Yeah, as Shiwei suggested below, .test is probably a better option for us.

shizhMSFT commented 2 weeks ago

@JeyJeyGao RFC 2606 defines some safer domains (see https://datatracker.ietf.org/doc/html/rfc2606#section-2)

JeyJeyGao commented 2 weeks ago

@JeyJeyGao RFC 2606 defines some safer domains (see https://datatracker.ietf.org/doc/html/rfc2606#section-2)

Updated the URL to be localhost.test

shizhMSFT commented 2 weeks ago

What about uncached.com?

JeyJeyGao commented 2 weeks ago

What about uncached.com?

Nice catch! Updated.