You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/11/19 16:28:23 UTC
[GitHub] [airflow] ashb opened a new pull request #19712: Rework webserver cli tests to not retest Gunicorn
ashb opened a new pull request #19712:
URL: https://github.com/apache/airflow/pull/19712
We were testing the webserver behaviour by actually invoking gunicorn -
this is both slow, and testing code that isn't ours.
Instead just test that we invoke gunicorn with the expected args
This should fix up the failures like we saw in https://github.com/apache/airflow/runs/4265409144?check_suite_focus=true
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#issuecomment-974280314
Oh nice, this takes the CLI tests from 226s on main to 130s here. Wasn't expecting that much diff.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#discussion_r753352566
##########
File path: tests/cli/commands/test_webserver_command.py
##########
@@ -386,63 +346,69 @@ def test_cli_webserver_shutdown_when_gunicorn_master_is_killed(self, _):
webserver_command.webserver(args)
assert ctx.value.code == 1
- def test_cli_webserver_debug(self):
- env = os.environ.copy()
- proc = psutil.Popen(["airflow", "webserver", "--debug"], env=env)
- time.sleep(3) # wait for webserver to start
- return_code = proc.poll()
- assert return_code is None, f"webserver terminated with return code {return_code} in debug mode"
- proc.terminate()
- assert -15 == proc.wait(60)
-
- def test_cli_webserver_access_log_format(self):
-
- # json access log format
- access_logformat = (
- "{\"ts\":\"%(t)s\",\"remote_ip\":\"%(h)s\",\"request_id\":\"%({"
- "X-Request-Id}i)s\",\"code\":\"%(s)s\",\"request_method\":\"%(m)s\","
- "\"request_path\":\"%(U)s\",\"agent\":\"%(a)s\",\"response_time\":\"%(D)s\","
- "\"response_length\":\"%(B)s\"} "
- )
- with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict(
- "os.environ",
- AIRFLOW__CORE__DAGS_FOLDER="/dev/null",
- AIRFLOW__CORE__LOAD_EXAMPLES="False",
- AIRFLOW__WEBSERVER__WORKERS="1",
- ):
- access_logfile = f"{tmpdir}/access.log"
- # Run webserver in foreground and terminate it.
+ def test_cli_webserver_debug(self, app):
+ with mock.patch.object(webserver_command, 'create_app') as create_app, mock.patch.object(
+ app, 'run'
+ ) as app_run:
+ create_app.return_value = app
- proc = subprocess.Popen(
+ args = self.parser.parse_args(
+ [
+ "webserver",
+ "--debug",
+ ]
+ )
+ webserver_command.webserver(args)
+
+ app_run.assert_called_with(
+ debug=True,
+ use_reloader=False,
+ port=8080,
+ host='0.0.0.0',
+ ssl_context=None,
+ )
+
+ def test_cli_webserver_args(self):
+ access_logformat = "custom_log_format"
+
Review comment:
```suggestion
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#issuecomment-974281158
All the standard parts passed, no point waiting on Helm tests for this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#discussion_r753360665
##########
File path: tests/cli/commands/test_webserver_command.py
##########
@@ -386,63 +346,68 @@ def test_cli_webserver_shutdown_when_gunicorn_master_is_killed(self, _):
webserver_command.webserver(args)
assert ctx.value.code == 1
- def test_cli_webserver_debug(self):
- env = os.environ.copy()
- proc = psutil.Popen(["airflow", "webserver", "--debug"], env=env)
- time.sleep(3) # wait for webserver to start
- return_code = proc.poll()
- assert return_code is None, f"webserver terminated with return code {return_code} in debug mode"
- proc.terminate()
- assert -15 == proc.wait(60)
-
- def test_cli_webserver_access_log_format(self):
-
- # json access log format
- access_logformat = (
- "{\"ts\":\"%(t)s\",\"remote_ip\":\"%(h)s\",\"request_id\":\"%({"
- "X-Request-Id}i)s\",\"code\":\"%(s)s\",\"request_method\":\"%(m)s\","
- "\"request_path\":\"%(U)s\",\"agent\":\"%(a)s\",\"response_time\":\"%(D)s\","
- "\"response_length\":\"%(B)s\"} "
- )
- with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict(
- "os.environ",
- AIRFLOW__CORE__DAGS_FOLDER="/dev/null",
- AIRFLOW__CORE__LOAD_EXAMPLES="False",
- AIRFLOW__WEBSERVER__WORKERS="1",
- ):
- access_logfile = f"{tmpdir}/access.log"
- # Run webserver in foreground and terminate it.
+ def test_cli_webserver_debug(self, app):
+ with mock.patch.object(webserver_command, 'create_app') as create_app, mock.patch.object(
+ app, 'run'
+ ) as app_run:
+ create_app.return_value = app
- proc = subprocess.Popen(
+ args = self.parser.parse_args(
[
- "airflow",
"webserver",
- "--access-logfile",
- access_logfile,
- "--access-logformat",
- access_logformat,
+ "--debug",
]
)
- assert proc.poll() is None
+ webserver_command.webserver(args)
+
+ app_run.assert_called_with(
+ debug=True,
+ use_reloader=False,
+ port=8080,
+ host='0.0.0.0',
+ ssl_context=None,
+ )
- # Wait for webserver process
- time.sleep(10)
+ def test_cli_webserver_args(self):
Review comment:
```suggestion
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #19712:
URL: https://github.com/apache/airflow/pull/19712
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#discussion_r753350432
##########
File path: tests/cli/commands/test_webserver_command.py
##########
@@ -386,63 +346,69 @@ def test_cli_webserver_shutdown_when_gunicorn_master_is_killed(self, _):
webserver_command.webserver(args)
assert ctx.value.code == 1
- def test_cli_webserver_debug(self):
- env = os.environ.copy()
- proc = psutil.Popen(["airflow", "webserver", "--debug"], env=env)
- time.sleep(3) # wait for webserver to start
- return_code = proc.poll()
- assert return_code is None, f"webserver terminated with return code {return_code} in debug mode"
- proc.terminate()
- assert -15 == proc.wait(60)
-
- def test_cli_webserver_access_log_format(self):
-
- # json access log format
- access_logformat = (
- "{\"ts\":\"%(t)s\",\"remote_ip\":\"%(h)s\",\"request_id\":\"%({"
- "X-Request-Id}i)s\",\"code\":\"%(s)s\",\"request_method\":\"%(m)s\","
- "\"request_path\":\"%(U)s\",\"agent\":\"%(a)s\",\"response_time\":\"%(D)s\","
- "\"response_length\":\"%(B)s\"} "
- )
- with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict(
- "os.environ",
- AIRFLOW__CORE__DAGS_FOLDER="/dev/null",
- AIRFLOW__CORE__LOAD_EXAMPLES="False",
- AIRFLOW__WEBSERVER__WORKERS="1",
- ):
- access_logfile = f"{tmpdir}/access.log"
- # Run webserver in foreground and terminate it.
+ def test_cli_webserver_debug(self, app):
+ with mock.patch.object(webserver_command, 'create_app') as create_app, mock.patch.object(
+ app, 'run'
+ ) as app_run:
+ create_app.return_value = app
- proc = subprocess.Popen(
+ args = self.parser.parse_args(
+ [
+ "webserver",
+ "--debug",
+ ]
+ )
+ webserver_command.webserver(args)
+
+ app_run.assert_called_with(
+ debug=True,
+ use_reloader=False,
+ port=8080,
+ host='0.0.0.0',
+ ssl_context=None,
+ )
+
+ def test_cli_webserver_args(self):
+ access_logformat = "custom_log_format"
+
+ with mock.patch("subprocess.Popen") as Popen, mock.patch.object(webserver_command, 'GunicornMonitor'):
+ args = self.parser.parse_args(
[
- "airflow",
"webserver",
- "--access-logfile",
- access_logfile,
"--access-logformat",
access_logformat,
+ "--pid",
+ "/tmp/x.pid",
]
)
- assert proc.poll() is None
+ webserver_command.webserver(args)
- # Wait for webserver process
- time.sleep(10)
-
- proc2 = subprocess.Popen(["curl", "http://localhost:8080"])
- proc2.wait(10)
- try:
- with open(access_logfile) as file:
- log = json.loads(file.read())
- assert '127.0.0.1' == log.get('remote_ip')
- assert len(log) == 9
- assert 'GET' == log.get('request_method')
-
- except OSError:
- print("access log file not found at " + access_logfile)
-
- # Terminate webserver
- proc.terminate()
- # -15 - the server was stopped before it started
- # 0 - the server terminated correctly
- assert proc.wait(60) in (-15, 0)
- self._check_processes()
+ Popen.assert_called_with(
+ [
+ sys.executable,
+ '-m',
+ 'gunicorn',
+ '--workers',
+ '4',
+ '--worker-class',
+ 'sync',
+ '--timeout',
+ '120',
+ '--bind',
+ '0.0.0.0:8080',
+ '--name',
+ 'airflow-webserver',
+ '--pid',
+ '/tmp/x.pid',
+ '--config',
+ 'python:airflow.www.gunicorn_config',
+ '--access-logfile',
+ '-',
+ '--error-logfile',
+ '-',
+ '--access-logformat',
+ access_logformat,
Review comment:
```suggestion
'custom_log_format',
```
##########
File path: tests/cli/commands/test_webserver_command.py
##########
@@ -386,63 +346,69 @@ def test_cli_webserver_shutdown_when_gunicorn_master_is_killed(self, _):
webserver_command.webserver(args)
assert ctx.value.code == 1
- def test_cli_webserver_debug(self):
- env = os.environ.copy()
- proc = psutil.Popen(["airflow", "webserver", "--debug"], env=env)
- time.sleep(3) # wait for webserver to start
- return_code = proc.poll()
- assert return_code is None, f"webserver terminated with return code {return_code} in debug mode"
- proc.terminate()
- assert -15 == proc.wait(60)
-
- def test_cli_webserver_access_log_format(self):
-
- # json access log format
- access_logformat = (
- "{\"ts\":\"%(t)s\",\"remote_ip\":\"%(h)s\",\"request_id\":\"%({"
- "X-Request-Id}i)s\",\"code\":\"%(s)s\",\"request_method\":\"%(m)s\","
- "\"request_path\":\"%(U)s\",\"agent\":\"%(a)s\",\"response_time\":\"%(D)s\","
- "\"response_length\":\"%(B)s\"} "
- )
- with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict(
- "os.environ",
- AIRFLOW__CORE__DAGS_FOLDER="/dev/null",
- AIRFLOW__CORE__LOAD_EXAMPLES="False",
- AIRFLOW__WEBSERVER__WORKERS="1",
- ):
- access_logfile = f"{tmpdir}/access.log"
- # Run webserver in foreground and terminate it.
+ def test_cli_webserver_debug(self, app):
+ with mock.patch.object(webserver_command, 'create_app') as create_app, mock.patch.object(
+ app, 'run'
+ ) as app_run:
+ create_app.return_value = app
- proc = subprocess.Popen(
+ args = self.parser.parse_args(
+ [
+ "webserver",
+ "--debug",
+ ]
+ )
+ webserver_command.webserver(args)
+
+ app_run.assert_called_with(
+ debug=True,
+ use_reloader=False,
+ port=8080,
+ host='0.0.0.0',
+ ssl_context=None,
+ )
+
+ def test_cli_webserver_args(self):
+ access_logformat = "custom_log_format"
Review comment:
```suggestion
```
nit: probably not worth a separate variable?
##########
File path: tests/cli/commands/test_webserver_command.py
##########
@@ -386,63 +346,69 @@ def test_cli_webserver_shutdown_when_gunicorn_master_is_killed(self, _):
webserver_command.webserver(args)
assert ctx.value.code == 1
- def test_cli_webserver_debug(self):
- env = os.environ.copy()
- proc = psutil.Popen(["airflow", "webserver", "--debug"], env=env)
- time.sleep(3) # wait for webserver to start
- return_code = proc.poll()
- assert return_code is None, f"webserver terminated with return code {return_code} in debug mode"
- proc.terminate()
- assert -15 == proc.wait(60)
-
- def test_cli_webserver_access_log_format(self):
-
- # json access log format
- access_logformat = (
- "{\"ts\":\"%(t)s\",\"remote_ip\":\"%(h)s\",\"request_id\":\"%({"
- "X-Request-Id}i)s\",\"code\":\"%(s)s\",\"request_method\":\"%(m)s\","
- "\"request_path\":\"%(U)s\",\"agent\":\"%(a)s\",\"response_time\":\"%(D)s\","
- "\"response_length\":\"%(B)s\"} "
- )
- with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict(
- "os.environ",
- AIRFLOW__CORE__DAGS_FOLDER="/dev/null",
- AIRFLOW__CORE__LOAD_EXAMPLES="False",
- AIRFLOW__WEBSERVER__WORKERS="1",
- ):
- access_logfile = f"{tmpdir}/access.log"
- # Run webserver in foreground and terminate it.
+ def test_cli_webserver_debug(self, app):
+ with mock.patch.object(webserver_command, 'create_app') as create_app, mock.patch.object(
+ app, 'run'
+ ) as app_run:
+ create_app.return_value = app
- proc = subprocess.Popen(
+ args = self.parser.parse_args(
+ [
+ "webserver",
+ "--debug",
+ ]
+ )
+ webserver_command.webserver(args)
+
+ app_run.assert_called_with(
+ debug=True,
+ use_reloader=False,
+ port=8080,
+ host='0.0.0.0',
+ ssl_context=None,
+ )
+
+ def test_cli_webserver_args(self):
+ access_logformat = "custom_log_format"
+
+ with mock.patch("subprocess.Popen") as Popen, mock.patch.object(webserver_command, 'GunicornMonitor'):
+ args = self.parser.parse_args(
[
- "airflow",
"webserver",
- "--access-logfile",
- access_logfile,
"--access-logformat",
access_logformat,
Review comment:
```suggestion
"custom_log_format",
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#issuecomment-974226566
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #19712: Rework webserver cli tests to not retest Gunicorn
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19712:
URL: https://github.com/apache/airflow/pull/19712#discussion_r753352042
##########
File path: tests/cli/commands/test_webserver_command.py
##########
@@ -386,63 +346,69 @@ def test_cli_webserver_shutdown_when_gunicorn_master_is_killed(self, _):
webserver_command.webserver(args)
assert ctx.value.code == 1
- def test_cli_webserver_debug(self):
- env = os.environ.copy()
- proc = psutil.Popen(["airflow", "webserver", "--debug"], env=env)
- time.sleep(3) # wait for webserver to start
- return_code = proc.poll()
- assert return_code is None, f"webserver terminated with return code {return_code} in debug mode"
- proc.terminate()
- assert -15 == proc.wait(60)
-
- def test_cli_webserver_access_log_format(self):
-
- # json access log format
- access_logformat = (
- "{\"ts\":\"%(t)s\",\"remote_ip\":\"%(h)s\",\"request_id\":\"%({"
- "X-Request-Id}i)s\",\"code\":\"%(s)s\",\"request_method\":\"%(m)s\","
- "\"request_path\":\"%(U)s\",\"agent\":\"%(a)s\",\"response_time\":\"%(D)s\","
- "\"response_length\":\"%(B)s\"} "
- )
- with tempfile.TemporaryDirectory() as tmpdir, mock.patch.dict(
- "os.environ",
- AIRFLOW__CORE__DAGS_FOLDER="/dev/null",
- AIRFLOW__CORE__LOAD_EXAMPLES="False",
- AIRFLOW__WEBSERVER__WORKERS="1",
- ):
- access_logfile = f"{tmpdir}/access.log"
- # Run webserver in foreground and terminate it.
+ def test_cli_webserver_debug(self, app):
+ with mock.patch.object(webserver_command, 'create_app') as create_app, mock.patch.object(
+ app, 'run'
+ ) as app_run:
+ create_app.return_value = app
- proc = subprocess.Popen(
+ args = self.parser.parse_args(
+ [
+ "webserver",
+ "--debug",
+ ]
+ )
+ webserver_command.webserver(args)
+
+ app_run.assert_called_with(
+ debug=True,
+ use_reloader=False,
+ port=8080,
+ host='0.0.0.0',
+ ssl_context=None,
+ )
+
+ def test_cli_webserver_args(self):
+ access_logformat = "custom_log_format"
Review comment:
Yeah fair.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org