damccorm commented on pull request #16816: URL: https://github.com/apache/beam/pull/16816#issuecomment-1035694510
;tldr - go ahead and merge it if you're ok with that @lostluck thanks for the thoughts - FWIW I wasn't actually saying its the wrong approach because we should prefer a traditional dependency injection approach - I agree that is better in most cases (and probably support that as a repo blanket policy, it probably helps avoid pitfalls), but I'm not sure it was actually less clean or more brittle in that specific case. Regardless, I'm not really interested in doing a dependency injection type approach here so I'd just merge it. My primary reasoning for not testing that behavior was that by mocking CommandContext (and essentially mocking cmd.Output() by extension), I was not actually testing much - it was code coverage for the sake of code coverage, not in order to actually ensure we're making our code better. That would still apply with dependency injection. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
