[GitHub] [airflow] sekikn commented on a change in pull request #6401: [AIRFLOW-5732] Add unit test for the version command

[GitBox <gi...@apache.org>]

sekikn commented on a change in pull request #6401: [AIRFLOW-5732] Add unit test for the version command
URL: https://github.com/apache/airflow/pull/6401#discussion_r339555551
 
 

 ##########
 File path: tests/core.py
 ##########
 @@ -1795,6 +1796,12 @@ def test_variables(self):
         os.remove('variables2.json')
         os.remove('variables3.json')
 
+    def test_cli_version(self):
 
 Review comment:
   Thanks for the comment @mik-laj!
   
   > I'm afraid this test is in the wrong place. tests/cli/test_cli.py is a better file.
   
   The reason that I added the new test to test/core.py was it has many cli-related tests in its TestCli class. I have no objection to move the new test into tests/cli/test_cli.py, but is there any criteria to put a new cli-related test into it (or should we basically use tests/cli/test_cli.py in the future)?
   
   > I am also afraid of sys.stdout mocking. Python has a special mechanism for this.
   
   I didn't use `contextlib.redirect_stdout` because of concern for the compatibility with Python2, but I've just noticed [we're going to drop it on the master branch](https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-3+Drop+support+for+Python+2). I'll address it on another PR, thanks :)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services