Add more precise tests (tests for jinja params) for AirflowVersion #8505

mik-laj · 2020-04-21T22:45:46Z

Recently, I talked to @kaxil about testing our views. I suggested that we should test templates and flask views separately I have prepared a POC that shows how it can be done. I think this can increase confidence in these tests. We are currently testing only random words, but very much logic has no coverage and it is very difficult to make changes. If you want to make changes, you always have to test everything manually to see if the logic is behaving correctly. If we have automatic tests with a higher level of confidence then the changes will be easier to review and we will trust them more.

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

kaxil

I like it

kaxil · 2020-04-21T23:21:19Z

I recreated the Extra Links PR:

    @mock.patch('airflow.www.views.dagbag.get_dag')
    @mock.patch('airflow.www.views.BaseView.render_template')
    def test_extra_link_in_gantt_view(self, mock_render_template, get_dag_function):
        from tests.test_utils.mock_operators import Dummy2TestOperator
        dag = DAG('ex_dag', start_date=self.default_date)
        task = Dummy2TestOperator(task_id="some_dummy_task_2", dag=dag)
        get_dag_function.return_value = dag

        exec_date = dates.days_ago(2)
        start_date = datetime(2020, 4, 10, 2, 0, 0)
        end_date = exec_date + timedelta(seconds=30)

        with create_session() as session:
            for task in dag.tasks:
                ti = TaskInstance(task=task, execution_date=exec_date, state="success")
                ti.start_date = start_date
                ti.end_date = end_date
                session.add(ti)

        with self.app.app_context():
            mock_render_template.return_value = make_response("RESPONSE", 200)
            url = 'gantt?dag_id={}&execution_date={}'.format(dag.dag_id, exec_date)
            self.client.get(url, follow_redirects=True)

        mock_render_template.assert_called_once_with(
            'airflow/gantt.html',
            base_date=mock.ANY, dag=mock.ANY,
            data={
                'taskNames': ['some_dummy_task_2'],
                'tasks': [
                    {
                        'task_id': 'some_dummy_task_2',
                        'dag_id': 'ex_dag',
                        'execution_date': '2020-04-19T00:00:00+00:00',
                        'start_date': '2020-04-10T02:00:00+00:00', 'end_date': '2020-04-19T00:00:30+00:00',
                        'duration': None, 'state': 'success', 'try_number': 1,
                        'max_tries': 0, 'hostname': '', 'unixname': 'root',
                        'job_id': None, 'pool': 'default_pool', 'pool_slots': 1, 'queue': 'default',
                        'priority_weight': 1,
                        'operator': 'Dummy2TestOperator', 'queued_dttm': None,
                        'pid': None, 'executor_config': {},
                        'extraLinks': ['github', 'airflow']
                    }
                ], 'height': 50
            },
            demo_mode=False,
            execution_date=mock.ANY, form=mock.ANY, root=mock.ANY, scheduler_job=mock.ANY
        )

arghh we will need to find a way to test individual args such that order of elements in list or dict doesn't matter.

mik-laj · 2020-04-21T23:41:18Z

We can create context - catch_jinja_params() (similar to count_queries, contextlib.redirect_stdout(io.StringIO()) ) that will help us introduce these assertions into current tests. Thanks to this, we will still have assertions for the HTML code, but also more precise for the Jinja params only.

class TestVersionView(TestBase):
    def test_version(self):
        with catch_jinja_params() as jinja_params:
            resp = self.client.get('version', data=dict(
                username='test',
                password='test'
            ), follow_redirects=True)
            self.check_content_in_response('Version Info', resp)

        self.assertEqual(jinja_params.template_name, 'airflow/version.html')
        self.assertEqual(jinja_params.params, dict(
            airflow_version=version.version,
            git_version=mock.ANY,
            scheduler_job=mock.ANY,
            title='Version Info'
        ))

However, I only wanted to present the general concept in this PR.

kaxil · 2020-04-21T23:55:34Z

We can create context - catch_jinja_params() (similar to count_queries, contextlib.redirect_stdout(io.StringIO()) ) that will help us introduce these assertions into current tests. Thanks to this, we will still have assertions for the HTML code, but also more precise for the Jinja params only.

class TestVersionView(TestBase):
    def test_version(self):
        with catch_jinja_params() as jinja_params:
            resp = self.client.get('version', data=dict(
                username='test',
                password='test'
            ), follow_redirects=True)
            self.check_content_in_response('Version Info', resp)

        self.assertEqual(jinja_params.template_name, 'airflow/version.html')
        self.assertEqual(jinja_params.params, dict(
            airflow_version=version.version,
            git_version=mock.ANY,
            scheduler_job=mock.ANY,
            title='Version Info'
        ))

However, I only wanted to present the general concept in this PR.

I like the idea. Without that we would have to do it like the following which works too but less ideal:

    @mock.patch('airflow.www.views.dagbag.get_dag')
    @mock.patch('airflow.www.views.BaseView.render_template')
    def test_extra_link_in_gantt_view(self, mock_render_template, get_dag_function):
        from tests.test_utils.mock_operators import Dummy2TestOperator
        dag = DAG('ex_dag', start_date=self.default_date)
        Dummy2TestOperator(task_id="some_dummy_task_2", dag=dag)

        get_dag_function.return_value = dag

        exec_date = dates.days_ago(2)
        start_date = datetime(2020, 4, 10, 2, 0, 0)
        end_date = exec_date + timedelta(seconds=30)

        with create_session() as session:
            for task in dag.tasks:
                ti = TaskInstance(task=task, execution_date=exec_date, state="success")
                ti.start_date = start_date
                ti.end_date = end_date
                session.add(ti)

        with self.app.app_context():
            mock_render_template.return_value = make_response("RESPONSE", 200)
            url = 'gantt?dag_id={}&execution_date={}'.format(dag.dag_id, exec_date)
            self.client.get(url, follow_redirects=True, data=dict(
                username='test',
                password='test'
            ))

        self.assertCountEqual(
            mock_render_template.call_args[1]['data']['tasks'][0]['extraLinks'],
            ['airflow', 'github']
        )

kaxil · 2020-04-21T23:57:26Z

Converted to draft just so that we can discuss, I definitely like the approach :)

mik-laj · 2020-04-22T01:39:04Z

I aadded catch_jinja_params contextmanager.

kaxil · 2020-04-22T02:11:49Z

I like this, I tested it with:

    @mock.patch('airflow.www.views.dagbag.get_dag')
    def test_extra_link_in_gantt_view(self, get_dag_function):
        from tests.test_utils.mock_operators import Dummy2TestOperator
        dag = DAG('dag', start_date=self.default_date)
        Dummy2TestOperator(task_id="some_dummy_task_2", dag=dag)

        get_dag_function.return_value = dag

        exec_date = dates.days_ago(2)
        start_date = datetime(2020, 4, 10, 2, 0, 0)
        end_date = exec_date + timedelta(seconds=30)

        with create_session() as session:
            for task in dag.tasks:
                ti = TaskInstance(task=task, execution_date=exec_date, state="success")
                ti.start_date = start_date
                ti.end_date = end_date
                session.add(ti)

        url = 'gantt?dag_id={}&execution_date={}'.format(self.dag.dag_id, exec_date)
        with catch_jinja_params() as jinja_params:
            self.client.get(url, follow_redirects=True)

        self.assertEqual(jinja_params.template_name, 'airflow/gantt.html')
        self.assertCountEqual(
            jinja_params.template_params['data']['tasks'][0]['extraLinks'],
            ['airflow', 'github'])

and works fine. However, I have 1 question: Do we lose any Airflow related args/customizations because of side_effect=BaseView.render_template ??

mik-laj · 2020-04-22T02:27:17Z

@kaxil We don't lose anything because we replace the method in the BaseView class with mock, and then mock calls the original method. As a result, we have a wrapped(transparent) method. If any class inherits from BaseView, it must use super () for mock to be called. Please note that scheduler_job key appears in the tests, which is added in another class, i.e. not VersionView.

tests/test_utils/asserts.py

turbaszek

Love the idea!

potiuk · 2020-04-22T13:42:15Z

Love it as well.

ashb · 2020-04-23T06:53:46Z

Will look in more detail soon, but I don't think mocking is the best approach here - check out https://stackoverflow.com/a/40531281 or flask-testing module

ashb · 2020-04-23T07:56:54Z

Great feature though

ashb · 2020-04-23T09:24:09Z

tests/test_utils/asserts.py

And rather than a separate method/in utils, I would suggest making this a method on the already existing TestBase class -- after all it is only useful if you have a flask app, and TestBase sets that up for us.

mik-laj · 2020-04-23T10:46:28Z

@ashb we commonly use mock and this is a proven solution that we know well. However, I will look at this flask-testing. Thanks for the answer.

ashb · 2020-04-23T14:28:37Z

Using the suggested answer from the SO post has some advantages:

The context manager code is simpler.
The test code is more "direct" too (at least to my eyes):

    @mock.patch('airflow.www.views.dagbag.get_dag')
    @mock.patch('airflow.www.views.BaseView.render_template')
    def test_extra_link_in_gantt_view(self, mock_render_template, get_dag_function):
        from tests.test_utils.mock_operators import Dummy2TestOperator
        dag = DAG('ex_dag', start_date=self.default_date)
        task = Dummy2TestOperator(task_id="some_dummy_task_2", dag=dag)
        get_dag_function.return_value = dag

        exec_date = dates.days_ago(2)
        start_date = datetime(2020, 4, 10, 2, 0, 0)
        end_date = exec_date + timedelta(seconds=30)

        with create_session() as session:
            for task in dag.tasks:
                ti = TaskInstance(task=task, execution_date=exec_date, state="success")
                ti.start_date = start_date
                ti.end_date = end_date
                session.add(ti)

        with captured_templates(self.app) as jinja_params:
            url = 'gantt?dag_id={}&execution_date={}'.format(dag.dag_id, exec_date)
            self.client.get(url, follow_redirects=True)

        assert len(templates) == 1
        template, context = templates[0]
        assert template.name == 'index.html'

        assert context['data'] == {
                'taskNames': ['some_dummy_task_2'],
                'tasks': [
                    {
                        'task_id': 'some_dummy_task_2',
                        'dag_id': 'ex_dag',
                        'execution_date': '2020-04-19T00:00:00+00:00',
                        'start_date': '2020-04-10T02:00:00+00:00', 'end_date': '2020-04-19T00:00:30+00:00',
                        'duration': None, 'state': 'success', 'try_number': 1,
                        'max_tries': 0, 'hostname': '', 'unixname': 'root',
                        'job_id': None, 'pool': 'default_pool', 'pool_slots': 1, 'queue': 'default',
                        'priority_weight': 1,
                        'operator': 'Dummy2TestOperator', 'queued_dttm': None,
                        'pid': None, 'executor_config': {},
                        'extraLinks': ['github', 'airflow']
                    }
                ], 'height': 50
            }
        )

(untested though)

Add catch_jinja_params context manager

mik-laj · 2020-05-03T22:16:20Z

setup.py

 ############################################################################################################
 devel = [
    'beautifulsoup4~=4.7.1',
+    'blinker',


Required by flask: https://github.com/pallets/flask/blob/master/src/flask/signals.py#L13

mik-laj · 2020-05-03T22:17:37Z

@potiuk @ashb @kaxil @turbaszek I have updated PR. Now I use flask.signals instead of mock.

ashb

Nice, I think that is a bit cleaner.

Got idea on removing the "global" context vars too

kaxil · 2020-05-03T22:44:46Z

For future reference: https://flask.palletsprojects.com/en/1.1.x/signals/#subscribing-to-signals

mik-laj · 2020-05-06T00:56:11Z

@potiuk Do you have any comments? I rewrote the whole PR from scratch, so you can change your opinions.

potiuk · 2020-05-06T11:25:41Z

Looks great!

boring-cyborg bot added the area:webserver Webserver related Issues label Apr 21, 2020

mik-laj mentioned this pull request Apr 21, 2020

Fix Extra Links in Gannt View #8308

Merged

5 tasks

mik-laj requested a review from kaxil April 21, 2020 22:49

kaxil approved these changes Apr 21, 2020

View reviewed changes

mik-laj changed the title ~~Add more precise tests for AirflowVersion~~ [POC] Add more precise tests for AirflowVersion Apr 21, 2020

kaxil added the dont-merge label Apr 21, 2020

kaxil marked this pull request as draft April 21, 2020 23:56

kaxil removed the dont-merge label Apr 22, 2020

mik-laj changed the title ~~[POC] Add more precise tests for AirflowVersion~~ [POC] Add more precise tests (tests for jinja params) for AirflowVersion Apr 22, 2020

mik-laj commented Apr 22, 2020

View reviewed changes

tests/test_utils/asserts.py Outdated Show resolved Hide resolved

turbaszek approved these changes Apr 22, 2020

View reviewed changes

potiuk approved these changes Apr 22, 2020

View reviewed changes

mik-laj requested a review from ashb April 23, 2020 00:19

ashb reviewed Apr 23, 2020

View reviewed changes

Kamil Breguła and others added 4 commits May 3, 2020 23:43

Add more precise tests for AirflowVersion

abea764

fixup! Add more precise tests for AirflowVersion

04a5c55

Add catch_jinja_params context manager

fixup! fixup! Add more precise tests for AirflowVersion

0a006ee

Update tests/test_utils/asserts.py

fc3c7c1

Use signals instead of mock

cbca0ac

mik-laj force-pushed the unit-test-version branch from 65b351c to cbca0ac Compare May 3, 2020 22:10

mik-laj commented May 3, 2020

View reviewed changes

ashb approved these changes May 3, 2020

View reviewed changes

mik-laj marked this pull request as ready for review May 3, 2020 22:25

kaxil approved these changes May 3, 2020

View reviewed changes

mik-laj changed the title ~~[POC] Add more precise tests (tests for jinja params) for AirflowVersion~~ Add more precise tests (tests for jinja params) for AirflowVersion May 3, 2020

mik-laj requested review from potiuk and turbaszek May 3, 2020 23:04

Update requirements

5521186

turbaszek approved these changes May 4, 2020

View reviewed changes

potiuk approved these changes May 6, 2020

View reviewed changes

mik-laj merged commit d923b5b into apache:master May 6, 2020

mik-laj deleted the unit-test-version branch May 6, 2020 11:35

mik-laj mentioned this pull request May 6, 2020

Avoid color info in response of /dag_stats & /task_stats #8742

Merged

6 tasks

mik-laj mentioned this pull request Jul 7, 2020

Links to Stackdriver in WebUI #9083

Closed

Add more precise tests (tests for jinja params) for AirflowVersion #8505

Add more precise tests (tests for jinja params) for AirflowVersion #8505

Uh oh!

Conversation

mik-laj commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

kaxil commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil commented Apr 21, 2020

Uh oh!

mik-laj commented Apr 22, 2020

Uh oh!

kaxil commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

turbaszek left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Apr 22, 2020

Uh oh!

ashb commented Apr 23, 2020

Uh oh!

ashb commented Apr 23, 2020

Uh oh!

ashb Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

mik-laj commented Apr 23, 2020

Uh oh!

ashb commented Apr 23, 2020

Uh oh!

mik-laj May 3, 2020

Choose a reason for hiding this comment

Uh oh!

mik-laj commented May 3, 2020

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

kaxil commented May 3, 2020

Uh oh!

mik-laj commented May 6, 2020

Uh oh!

potiuk commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mik-laj commented Apr 21, 2020 •

edited

Loading

kaxil commented Apr 21, 2020 •

edited

Loading

mik-laj commented Apr 21, 2020 •

edited

Loading

kaxil commented Apr 21, 2020 •

edited

Loading

kaxil commented Apr 22, 2020 •

edited

Loading

mik-laj commented Apr 22, 2020 •

edited

Loading