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

Update docs and API usage in tests #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sean-
Copy link

@sean- sean- commented Dec 30, 2016

Test the public interface in a standalone _test.go file and then spin off the private implementation checks into an -impl_test.go file that has the necessary visibility. I didn't tease out the semantics of what was supposed to be happening in the TestFlush() call to see what could be split out. The intent of this PR is two fold:

  1. Document and use the public API in the docs and tests
  2. Test the implementation in a smaller set of tests that are scoped to just testing the private API. With the Flush() test it would be ideal to test in the public API test, however that would necessitate the creation of a ForceFlush() method or something like that, which is the only reason I didn't keep Flush() in the public API test function.

Feel free to reject this PR.

Style: `check` is not covered by the `flushmu` mutex.
- Standardize on using `New()` vs `NewCirconusMetrics()`
- Move most of the public tests to a dedicated `_test` package to test
  the public interface of the package.
- Stand up an `-impl_test.go` file to peak behind the curtains and test
  the implementation.
@maier
Copy link
Owner

maier commented Dec 30, 2016

Flush is part of the public interface. I don't understand the need to break up the testing file.

@sean-
Copy link
Author

sean- commented Dec 30, 2016

Flush() is public but it's not possible in the tests to test Flush() without poking the flushing struct member, which isn't exported.

https://github.com/maier/circonus-gometrics/pull/2/files#diff-a88c24be542e329037d63313aec2fdb2L305

It's not strictly necessary to split out, but it does help expose and document the canonical usage of the public API.

@maier
Copy link
Owner

maier commented Dec 30, 2016

If it is just to isolate the access of an internal flag flushing, then only that one test would need to be migrated. The other Flush() tests are all plain, basic use of the public interface. I guess I could go the pedantic, probably idiomatic, route of creating an additional endpoint with a timeout and using channels and go routines so that multiple flushes can be run simultaneously to verify internal behavior. Which, to me, seemed like tremendously fragile overkill to simply test two lines. It just seemed more straight-forward and robust to just set the flag and make the call to verify the two lines.

@sean-
Copy link
Author

sean- commented Dec 30, 2016 via email

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.

2 participants