potiuk commented on pull request #15634: URL: https://github.com/apache/airflow/pull/15634#issuecomment-832059665
> Thanks for your quick reaction. Let me address the individual fixes separately: > Just to give you a bit of context, we rely on automated testing heavily. And our philosophy is that with every single change we want to add unit tests so that the coverage grows and probability of regression goes down. Adding any change without tests almost by definition goes in the other direction, so almost any change that has no unit tests (and touches the python code) will end up with a request to add them. > * It am not aware of a practical way to test the sleep time in the main loop of the `_monitor_logging`. If you know a good way, of course I can do so. We usually mock any calls like that - example here where sleep command have been mocked: https://github.com/apache/airflow/blob/master/tests/cli/commands/test_webserver_command.py In this particular case we would like to add a test where we have at least two loops where state changes and sleep is called in-between. All that can be mocked rather easily. > * I for sure can test the addition of the state for logging. Coool > * Testing the different log level does not make much sense, I would say. Agree. > * Testing the check for missing events is also tricky, since the exception is only logged and not raised further. If there is a good way to test, that no exception is logged within the main loop, of course I can do that. I think mocking here should help as well. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
