Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Oct 29, 2024

Fix:

  • added mocked HTTP client for a test case of revocation package
  • replaced *.com with *.test to avoid mentioning a real domain in testing URL

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

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

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.

@JeyJeyGao
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@JeyJeyGao JeyJeyGao changed the title fix: test case for revocation fix: test cases for revocation Oct 29, 2024
@JeyJeyGao JeyJeyGao changed the title fix: test cases for revocation fix(test): test cases for revocation Oct 29, 2024
@JeyJeyGao JeyJeyGao changed the title fix(test): test cases for revocation fix(test): test cases with **example.com** URL Oct 29, 2024
@JeyJeyGao JeyJeyGao changed the title fix(test): test cases with **example.com** URL fix(test): test cases with *example.com* URL Oct 29, 2024
@JeyJeyGao JeyJeyGao changed the title fix(test): test cases with *example.com* URL fix(test): test cases with example.com URL Oct 29, 2024
@Two-Hearts
Copy link
Contributor

Two-Hearts commented Oct 29, 2024

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
Copy link
Contributor

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

@JeyJeyGao
Copy link
Contributor Author

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

Updated the URL to be localhost.test

@JeyJeyGao JeyJeyGao requested a review from Two-Hearts October 29, 2024 05:43
Two-Hearts
Two-Hearts previously approved these changes Oct 29, 2024
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT
Copy link
Contributor

What about uncached.com?

Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
Copy link
Contributor Author

What about uncached.com?

Nice catch! Updated.

@JeyJeyGao JeyJeyGao requested a review from Two-Hearts October 29, 2024 05:57
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao JeyJeyGao merged commit 4819e1d into notaryproject:main Nov 5, 2024
9 checks passed
@Two-Hearts Two-Hearts mentioned this pull request Dec 6, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants