Fix alwaysUseBid bid setting
#392
Conversation
| @@ -171,6 +171,36 @@ function getWinningBidTargeting() { | |||
| return winners; | |||
| } | |||
|
|
|||
| exports.getWinningBidTargeting = function() { | |||
There was a problem hiding this comment.
It seems you are exporting functions for the sole purpose of making them available to test. We've been following the general principle that we should test the public API of a module, that is the exports and ES2015 style named exports, but not the internal implementation of a module. Test coverage should show that comprehensive testing of the public API also covers internal implementation, or where it does not it indicates insufficient tests or cruft in the code. In any event, the export of functions in order to test should be avoided as it can be misleading as to the intent of the module.
There was a problem hiding this comment.
This was a helpful description of the test philosophy, thank you.
|
Thanks for contributing this fix, looks good, please see the comments about test strategy and remove unneeded exports, move tests to public API. |
|
We would like to include this in our pending release. We'll go ahead and merge as-is unless you have an update in the works. |
|
Thanks for the comments. I'll have a PR addressing them within the next few hours. |
|
@protonate These latest commits address your comments:
I also improved a few existing tests for |
|
@kmjennison |
|
@mkendall07 @kmjennison The conflict is due to Deal ID PR (merged) and this PR touching the same code so I've resolved conflicts, I'm going to get a review from @matthewlane to confirm Deal ID functionality, test and then merge this PR. |
|
This has been merged manually, closing, thanks for the fix @kmjennison. |
Fix for #389.