Skip to content

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Jun 15, 2020

CC: @Khrol

Related PR: #9250

The problem with the CI in this build was that it only tested the static tests. As no ".py" file was changed, CI skipped rest of the unit test. However on Master we run all the tests, hence Master failed with below error:

tests/www/test_views.py .............................F

_________ TestAirflowBaseViews.test_escape_in_tree_view_0_hello_world __________

a = (<tests.www.test_views.TestAirflowBaseViews testMethod=test_escape_in_tree_view_0_hello_world>,)

    @wraps(func)
    def standalone_func(*a):
>       return func(*(a + p.args), **p.kwargs)

/usr/local/lib/python3.6/site-packages/parameterized/parameterized.py:530: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/www/test_views.py:646: in test_escape_in_tree_view
    self.check_content_in_response(f'"conf":{{"abc":"{seralized_test_str}"}}', resp)
tests/www/test_views.py:177: in check_content_in_response
    self.assertIn(text, resp_html)
tests/www/test_views.py F

_________ TestAirflowBaseViews.test_escape_in_tree_view_1_hello_world __________

a = (<tests.www.test_views.TestAirflowBaseViews testMethod=test_escape_in_tree_view_1_hello_world>,)

    @wraps(func)
    def standalone_func(*a):
>       return func(*(a + p.args), **p.kwargs)

/usr/local/lib/python3.6/site-packages/parameterized/parameterized.py:530: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/www/test_views.py:646: in test_escape_in_tree_view
    self.check_content_in_response(f'"conf":{{"abc":"{seralized_test_str}"}}', resp)
tests/www/test_views.py:177: in check_content_in_response
    self.assertIn(text, resp_html)
E   AssertionError: '"conf":{"abc":"hello\\\\u0027world"}' not found in '\n\n\n\n\n\n\n\n\n\n 

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)
  • Target Github ISSUE in description if exists
  • 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.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jun 15, 2020
@mik-laj mik-laj mentioned this pull request Jun 15, 2020
6 tasks
@mik-laj mik-laj requested review from ashb, kaxil and turbaszek June 15, 2020 13:10
var devicePixelRatio = window.devicePixelRatio || 1;
// JSON.parse is faster for large payloads than an object literal (because the JSON grammer is simpler!)
var data = JSON.parse({{ data|tojson }});
var data = {{ data }};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not remove JSON.parse with tojson due to performance problems (see comment above).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. This was added for a reason. As it says in the comment directly before this line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I will correct it. Thanks for your vigilance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added JSON.parse.

@ashb ashb changed the title Fix failing master - Invalid assertion in conf Fix failing tests from #9250 - Invalid assertion in conf Jun 15, 2020
@ashb ashb changed the title Fix failing tests from #9250 - Invalid assertion in conf Fix failing tests from #9250 Jun 15, 2020
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not remove the JSON.parse

@ashb
Copy link
Member

ashb commented Jun 15, 2020

@mik-laj @Khrol Would one of you be able to update the test runner to run tests in case of template changes too please?

@mik-laj mik-laj requested a review from ashb June 15, 2020 13:24
@mik-laj
Copy link
Member Author

mik-laj commented Jun 15, 2020

@mik-laj @Khrol Would one of you be able to update the test runner to run tests in case of template changes too please?

I'm too busy to do it now. @Khrol Can you help with it?

@mik-laj mik-laj merged commit 2c18a3f into apache:master Jun 15, 2020
@mik-laj mik-laj deleted the fix-master-conf branch June 15, 2020 14:23
@mik-laj mik-laj mentioned this pull request Jun 15, 2020
6 tasks
kaxil pushed a commit that referenced this pull request Jun 23, 2020
(cherry-picked from 2c18a3f)
kaxil added a commit that referenced this pull request Jun 23, 2020
potiuk pushed a commit that referenced this pull request Jun 29, 2020
(cherry-picked from 2c18a3f)
kaxil pushed a commit that referenced this pull request Jul 1, 2020
(cherry-picked from 2c18a3f)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
(cherry-picked from 2c18a3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants