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

Make Geocoding testable from desktop unit test projects #81

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
3 participants
@kzu
Member

kzu commented Mar 14, 2018

This showcases how a minimal change in the netstandard version of the
API can trivially allow for fully testable API invocations without
incurring in any of the run-time performance compromises of switching
the platform implementations to use either unnecessary interface/virtual
calls or even instance method call overhead ('cause of the null reference
check that's otherwise not necessary).

This is an effective compromise that covers the majority of the testing
scenarios (unit testing from view models down) while still preserving
the desired run-time characteristics of the project.

Description of Change

Tweak just the netstandard version of an API to make it trivially
testable from desktop unit tests.

Bugs Fixed

The overwhelming feedback that un-testable APIs are not cool ;)

Also, my personal belief that forcing everyone to invent their own
abstractions and achieving the same through unnecessary indirections
that do impact ultimate run-time performance is bad for both
our users (developers) as well as their customers (users of their apps).

API Changes

Added:

  • IGeocoding (as an example only, it's not strictly required), only to the
    netstandard Geocoding
  • Geocoding.Current, to allow replacing the implementation with a fake/mock

Behavioral Changes

No behavioral changes whatsoever are introduced in the platform code. The
netstandard code passes all existing unit tests too, since by default it will still
throw the same exception as before, unless a test overrides the implementation.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@kzu kzu added the DO-NOT-MERGE label Mar 14, 2018

Make Geocoding testable from desktop unit test projects
This showcases how a minimal change in the netstandard version of the
API can trivially allow for fully testable API invocations without
incurring in *any* of the run-time performance compromises of switching
the platform implementations to use either unnecessary interface/virtual
calls or even instance method call overhead ('cause of the null reference
check that's otherwise not necessary).

This is an effective compromise that covers the majority of the testing
scenarios (unit testing from view models down) while still preserving
the desired run-time characteristics of the project.

@kzu kzu force-pushed the kzu:testable branch from 7b7f021 to 227e9d9 Mar 14, 2018

@jamesmontemagno

This comment has been minimized.

Collaborator

jamesmontemagno commented Mar 14, 2018

What will happen here if someone was to create a device runner with xunit or nunit tests? I assume it will not use the mocks and still use our implementation?

@kzu

This comment has been minimized.

Member

kzu commented Mar 14, 2018

Yes, of course. In fact, they will not see the API at all in their device tests, so it will fail at compile time, since for devices, the Caboodle will always run at "full speed" with no abstractions whatesoever. At that point they will have to find a more creative workaround (like the suggested usage of something that does IL rewriting to allow faking static methods).

It's a trade-off, but one we can more easily get buy in from @migueldeicaza I'd say ;)

@kzu

This comment has been minimized.

Member

kzu commented Mar 14, 2018

And I'm calling this pattern "test and switch", just so you know ;)

Allow parallel tests to set their own mocks
By storing the Current instance in AsyncLocal, it can be
properly set per-thread and propagated automatically across
async calls.

NOTE: since AsyncLocal<T> only exists in netstandard2.0, I
just removed netstandard1.0 ;). Of course, this is just to
show how we could do this, but if we support this for real,
we'd need to have different implementations for ns1.0 vs
ns2.0.

@kzu kzu force-pushed the kzu:testable branch from 06a7a8a to 85da8b4 Mar 14, 2018

@ghuntley

This comment has been minimized.

ghuntley commented Mar 14, 2018

Thank-you @kzu

@kzu

This comment has been minimized.

Member

kzu commented Mar 15, 2018

I'll take it as an "I think it's a good idea" @ghuntley ;)

@jamesmontemagno

This comment has been minimized.

Collaborator

jamesmontemagno commented Mar 16, 2018

Thanks for the PR kzu, we have analyzed it and like the idea, but not ready to implement it, but will keep it in mind for the future. We have documented our approach for developers in our README.

@mattleibow mattleibow added this to Closed in v0.5.0-preview Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment