Split Formatter into ConsoleFormatter and LogFileFormatter; use DelegatingFormatter to send logging events to both #38
Conversation
The purpose of this class is to forward logging methods to 1 or more other formatters.
This makes SSHKit::Formatter::Airbrussh a subclass of Airbrussh::DelegatingFormatter, which will facilitate future refactoring. The Configuration object now gains the responsibility of being a factory for the formatter objects that will be delegated to.
This allows the `Airbrussh::Formatter` to once again be the primary entry point to airbrussh.
| @@ -12,6 +16,26 @@ def initialize | |||
| self.command_output = false | |||
| end | |||
|
|
|||
| def banner_message | |||
There was a problem hiding this comment.
This is good - agreed about the old banner_message behaviour being a bug
|
I think the formatter changes look good. One thing I am uncomfortable with is the fact that all of the assertions on the log file content have been removed. I think it's really valuable to have an integration test for Airbrussh where we assert on both the console output and the file output for a selection of commands with a real backend. This gives me real confidence that the Airbrussh as a whole is working as intended when refactoring, and when integrating against new SSHKit versions. The reason why I (painstakingly!) added all of the log file output tests was because it was really hard to debug why particular log files were or weren't appearing across different versions of SSHKit. Removing these means that it will be hard to spot when the integration with the log file changes or breaks due to SSHKit changes. I think the testing in Airbrussh is more comprehensive than it is in SSHKit, so if we keep the integration test we will likely catch changes / regressions which are missed in SSHKit. In the PR as it currently stands, there is quite a bit of unused code related to the partially removed log file testing. The Deleting the above methods would mean that the color methods and the However, my preference would be to reinstate the integration test asserting on both the console and the log file. What are your thoughts on unit vs integration testing? |
|
@robd I am pretty confident in the current testing, because even though the console and log file formatters aren't tested together, they are both exercised individually, and there is also good coverage for the delegating formatter that joins the two. I removed most of the log file assertions, because now that we are simply delegating to I was also motivated by your comment:
But, if like you said, the log file output tests are critical coverage that doesn't already exist in SSHKit itself, then maybe I should reinstate them. In that case I will probably put them in All of this aside, I do agree that we should have at least a "sanity check" or "smoke test" integration-style test that drives the entire thing from |
Yes, I didn't mean to imply that the testing of Airbrussh code isn't complete. I understand that the log file tests are testing the Pretty formatter and duplicate, to some extent, the Pretty formatter tests in SSHKit. But at the moment, they are the only tests which provide acceptance test type of coverage, ie that the user is getting all the lines they need to be able to see what's going on with their deployment. When I said this:
I can see that my wording here wasn't very clear and this is unhelpful - I'm sorry about that. I didn't mean to imply that we no longer need integration / acceptance tests (IMHO). I assumed that they would be added to SSHKit, in some form, along with the support for multiple formatters, and could then be removed from Airbrussh. I guess this is why I was a bit surprised to see the coverage removed completely as part of this PR. I do think that the Airbrussh log output coverage is more comprehensive than what is in SSHKit. The main thing I remember is that SSHKit doesn't test command failure at all. Also, the testing in SSHKit is at the unit level not at the acceptance test level. Personally, I really like being able to see what will appear on the console and log file for the major use cases:
At the moment, for me, the Formatter test is a sort of user level specification of what the whole SSHKit / Airbrussh combo should do when different commands are run. I think it is a disadvantage to have this spread over 2 tests (ie spread over a ConsoleFormatterTest and LogFileFormatterTest) and have to work out whether the cases in each test are the same / different, duplicate the test setup etc. For unit tests, I'd prefer to have simple tests which don't go through a backend or run everything as rake tasks. I envisage these would call the actual methods on the formatters and assert on an individual methods (eg write, log_command_start etc). But I'm not sure if a ConsoleFormatter unit test is needed if the acceptance tests are comprehensive. I definitely don't think we need to unit test the I do think that the code separation you've done is a big improvement and I think that it's much clearer what's going on now so I am definitely positive about this change. I realise that you have already done a huge amount to get the PR this far - thank you for considering my feedback! If you would like me to look at reinstating the acceptance test I would be happy to do that. It's quite late here now (I'm in the UK), so I probably won't have a chance to look at this until tomorrow lunchtime, but let me know what you think. |
|
@robd Thanks for your detailed writeup on the issue. I've reinstated the acceptance tests. Next time you have a chance, take a look and let me know if I've missed anything. |
| fatal error warn info debug log | ||
| log_command_start log_command_data log_command_exit | ||
| ) | ||
| DUP_AND_FORWARD_METHODS = %w(<< write) |
There was a problem hiding this comment.
This is great to handle the duplication concern here
|
Thank you very much for reinstating the acceptance style test. I've looked through again and I can't see anything else so all good to merge from my perspective. |
|
@robd Thanks for reviewing! I'll merge this. |
Split Formatter into ConsoleFormatter and LogFileFormatter; use DelegatingFormatter to send logging events to both
This PR replaces #36 and contains the complete set of refactoring originally proposed in #24.
As before, this adds
DelegatingFormatter, which forwards logging methods to 1 or more other formatters. The former responsibilities ofAirbrussh::Formatterhave now been split into two new classes:ConsoleFormatterandLogFileFormatter. All of these new class are independently unit tested.With these changes,
Airbrussh::Formatterbecomes a trivial subclass ofDelegatingFormatter, using the configuration to determine what formatters to forward to.SSHKit::Formatter::Airbrusshremains an alias forAirbrussh::Formatter. From the end-user's perspective, nothing should appear different.There is one small behavior change. Before, if no log file was configured, the automatic banner message would disappear entirely. Now the banner simply says
Using airbrussh format.(i.e. without the sentence about the log file). I consider the old behavior to be a bug.@robd Your review and suggestions would be appreciated, as always!