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

apple dns: add fake api test suite #13780

Merged
merged 4 commits into from
Oct 28, 2020
Merged

apple dns: add fake api test suite #13780

merged 4 commits into from
Oct 28, 2020

Conversation

junr03
Copy link
Member

@junr03 junr03 commented Oct 27, 2020

Commit Message: apple dns - add fake api test suite
Additional Description: use a singleton to inject a mock class for Apple's API calls during testing. This allows for finer control for error paths and callback firing.
Risk Level: low - more test coverage
Testing: adds new unit test suite

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Oct 27, 2020

@mattklein123 I am pleased about how this turned out! As I was writing tests I thought about a couple behaviors that are not necessarily wrong or right but wanted to hear what you thought about them -- I have marked them with and inline comment that starts with DISCUSSION.

Unfortunately running coverage with bazel in MacOS is a mess (bazelbuild/bazel#10457) so I was unable to run an actual coverage report. I inspected the code and believe I have covered all branches, but if you spot anything you'd like to see a test on lmk.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks very nice. Excited to see better tests here. Some small comments and also needs a format fix.

/wait

source/common/network/apple_dns_impl.cc Outdated Show resolved Hide resolved
source/common/network/apple_dns_impl.cc Outdated Show resolved Hide resolved
test/mocks/network/dns_service.h Outdated Show resolved Hide resolved
test/common/network/apple_dns_impl_test.cc Outdated Show resolved Hide resolved
Jose Nino added 3 commits October 27, 2020 18:07
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
fmt
Signed-off-by: Jose Nino <jnino@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice!

@junr03 junr03 merged commit 5b97d80 into envoyproxy:master Oct 28, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 30, 2020
* master: (83 commits)
  tls: Typesafe tls slots (envoyproxy#13789)
  docs(example): Correct URL for caching example page (envoyproxy#13810)
  [fuzz] Made health check fuzz more efficient (envoyproxy#13747)
  rtds: properly scope rtds stats (envoyproxy#13764)
  http: fixing a bug with IPv6 hosts (envoyproxy#13798)
  connection: Remember transport socket read resumption requests and replay them when re-enabling read. (envoyproxy#13772)
  network: adding some accessors for ALPN work. (envoyproxy#13785)
  docs: added a step about how to handle platform specific extensions (envoyproxy#13759)
  Fix identation in ip transparency code snippet (envoyproxy#13743)
  wasm: enable WAVM's stack unwinding feature (envoyproxy#13792)
  log: set route name for direct response (envoyproxy#13683)
  Use nghttp2 as external dependsncy in protocol_constraints_lib (envoyproxy#13763)
  [Windows] Update windows dev docs (envoyproxy#13741)
  cel: patch thread safety issue (envoyproxy#13739)
  Windows: Fix ssl_socket_test (envoyproxy#13264)
  apple dns: add fake api test suite (envoyproxy#13780)
  overload: scale selected timers in response to load (envoyproxy#13475)
  examples: Add dynamic configuration (control plane) sandbox (envoyproxy#13746)
  Removed exception in getResponseStatus() (envoyproxy#13314)
  network: add timeout for transport connect (envoyproxy#13610)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
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.

3 participants