Upgrades isort webtest and pylint quotes #6938

NishealJ · 2019-06-15T22:10:22Z

Explanation

This PR upgrades Upgrades isort webtest and pylint quotes.
To see the library status please visit here
Please visit here for the testing doc

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

codecov · 2019-06-16T04:32:31Z

Codecov Report

Merging #6938 into develop will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           develop    #6938   +/-   ##
========================================
  Coverage    95.73%   95.73%           
========================================
  Files          371      371           
  Lines        51604    51604           
========================================
  Hits         49404    49404           
  Misses        2200     2200

Impacted Files	Coverage Δ
core/tests/gae_suite.py	`74.41% <ø> (ø)`	⬆️
core/controllers/base.py	`100% <100%> (ø)`	⬆️
core/controllers/base_test.py	`98.33% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cdd42c...2185c32. Read the comment docs.

codecov · 2019-06-16T04:32:31Z

Codecov Report

Merging #6938 into develop will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           develop    #6938   +/-   ##
========================================
  Coverage    95.73%   95.73%           
========================================
  Files          371      371           
  Lines        51604    51604           
========================================
  Hits         49404    49404           
  Misses        2200     2200

Impacted Files	Coverage Δ
core/tests/gae_suite.py	`74.41% <ø> (ø)`	⬆️
core/controllers/base.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cdd42c...f2fe911. Read the comment docs.

NishealJ · 2019-06-16T07:26:37Z

Hi @apb7,
I've tested this and here is the testing doc.
I'm removing the labels Thanks!

apb7

Hi @NishealJ, left some comments. PTAL!

apb7 · 2019-06-16T15:01:07Z

core/controllers/base.py

@@ -339,8 +339,9 @@ def render_template(self, filepath, iframe_restriction='DENY'):
        self.response.headers['X-Xss-Protection'] = '1; mode=block'

        if iframe_restriction is not None:
-            if iframe_restriction in ['SAMEORIGIN', 'DENY']:
-                self.response.headers['X-Frame-Options'] = iframe_restriction
+            if iframe_restriction in [str('SAMEORIGIN'), str('DENY')]:


Why these changes?

Hi @apb7,
The latest version of web test supports latin 1 string formats while sending a request.
If they are not typecasted into str they where moved to 500 error and the backend tests fails view here

Can you please point me to some documentation for webtest re this change?
Also, you can drop the str here since you're already converting iframe_restriction to a string below.

Hi @apb7,

if we dont str() them, they throw an error AssertionError: Header names must be latin1 string (not Py2 unicode or Py3 bytes type). i've read more about it here and here, seems like HTTP response headers should be a native strings - byte strings in Python 2 and unicode strings in Python 3, here is the issue raised in webtest repo for the same

Can you please point to the changelog of the version which mentions this change? There must be some documentation for this.
And I think you missed this:

Also, you can drop the str here since you're already converting iframe_restriction to a string below.

I suggest not to blindly put up str everywhere without first understanding the need.

Hi @apb7 I've made it tedious and justifying please PTAL Thanks!

Here is the changelog of wbetest they haven't mentioned in the changelog but the changes in the version below are notable for the cause

2.0.11 : Depend on unittest2 only for Python versions lower than 2.7 [iElectric] 2.0.28 : Fixed #119: Assertion error should be raised when you have non-string response header

Also similar notes can be found in requests changelog here

2.14.0 Breaking changes: Much improved handling of non-ASCII Location header values in redirects. Fewer UnicodeDecodeErrors are encountered on Python 2, and Python 3 now correctly understands that Latin-1 is unlikely to be the correct encoding.

Reading through the above docs and here as well looks like the application should return the native strings for headers and Python 2 build-in WSGI server don't help us in encoding/decoding and it must must be handled by the application itself in Python 2 whereas the case is different in Python 3.

apb7 · 2019-06-16T15:02:09Z

core/controllers/base_test.py

@@ -831,8 +831,8 @@ def test_responses_with_valid_iframe_restriction(self):
        self.get_html_response('/mock')

        response = self.get_html_response(
-            '/mock', params={'iframe_restriction': 'DENY'})
-        self.assertEqual(response.headers['X-Frame-Options'], 'DENY')
+            '/mock', params={str('iframe_restriction'): str('DENY')})


Why these changes?

Hi @apb7, This is also for a similar reason I mentioned above

package-lock.json

apb7 · 2019-06-17T09:48:05Z

core/controllers/base.py

@@ -339,8 +339,9 @@ def render_template(self, filepath, iframe_restriction='DENY'):
        self.response.headers['X-Xss-Protection'] = '1; mode=block'

        if iframe_restriction is not None:
-            if iframe_restriction in ['SAMEORIGIN', 'DENY']:
-                self.response.headers['X-Frame-Options'] = iframe_restriction
+            if iframe_restriction in [str('SAMEORIGIN'), str('DENY')]:


Can you please point me to some documentation for webtest re this change?
Also, you can drop the str here since you're already converting iframe_restriction to a string below.

apb7 · 2019-06-17T09:49:11Z

core/controllers/base_test.py

-            '/mock', params={'iframe_restriction': 'DENY'})
-        self.assertEqual(response.headers['X-Frame-Options'], 'DENY')
+            '/mock', params={str('iframe_restriction'): str('DENY')})
+        self.assertEqual(response.headers[str('X-Frame-Options')], str('DENY'))

        response = self.get_html_response(
            '/mock', params={'iframe_restriction': 'SAMEORIGIN'})


Why do these lines work then (that is, without the conversion)?

Hi @apb7,

i also thought about it but seeing the tests failing here it clearly mentioned that 'u'DENY' is not a valid latin1 string' that's why I only str() it and tested again

I don't think that this is valid reasoning. I agree that only a particular test fails but we should be clear what's happening behind the scenes and why the other test does not fail.

Hi @apb7, I've updated the changes with reason PTAL Thanks!

apb7 · 2019-06-17T09:50:17Z

Hi @NishealJ, ptal! Thanks!

NishealJ · 2019-06-18T12:28:45Z

Hi @NishealJ, ptal! Thanks!
Hi @abp7 I've addressed your comments Thanks!

apb7

Hi @NishealJ, ptal!

apb7 · 2019-06-18T12:50:53Z

core/controllers/base.py

@@ -339,8 +339,9 @@ def render_template(self, filepath, iframe_restriction='DENY'):
        self.response.headers['X-Xss-Protection'] = '1; mode=block'

        if iframe_restriction is not None:
-            if iframe_restriction in ['SAMEORIGIN', 'DENY']:
-                self.response.headers['X-Frame-Options'] = iframe_restriction
+            if iframe_restriction in [str('SAMEORIGIN'), str('DENY')]:


Can you please point to the changelog of the version which mentions this change? There must be some documentation for this.
And I think you missed this:

Also, you can drop the str here since you're already converting iframe_restriction to a string below.

I suggest not to blindly put up str everywhere without first understanding the need.

apb7 · 2019-06-18T12:52:15Z

core/controllers/base_test.py

-            '/mock', params={'iframe_restriction': 'DENY'})
-        self.assertEqual(response.headers['X-Frame-Options'], 'DENY')
+            '/mock', params={str('iframe_restriction'): str('DENY')})
+        self.assertEqual(response.headers[str('X-Frame-Options')], str('DENY'))

        response = self.get_html_response(
            '/mock', params={'iframe_restriction': 'SAMEORIGIN'})


I don't think that this is valid reasoning. I agree that only a particular test fails but we should be clear what's happening behind the scenes and why the other test does not fail.

apb7

LGTM, thanks for the explanation, @NishealJ! Please revert the changes in package-lock.json. Also, @nithusha21, @seanlip and @vojtechjelinek, ptal as code owners. Thanks!

seanlip · 2019-06-19T08:56:39Z

I won't be reviewing until I see a testing doc with links to the changelogs for the various libraries and an explanation that analyzes any breaking changes and calls out what is being done about them.

(@NishealJ, I think I've asked for this before -- please take note of this for all library upgrades.)

NishealJ · 2019-06-19T11:23:19Z

I won't be reviewing until I see a testing doc with links to the changelogs for the various libraries and an explanation that analyzes any breaking changes and calls out what is being done about them.

(@NishealJ, I think I've asked for this before -- please take note of this for all library upgrades.)

Hi @seanlip, sorry for the issue.
I've updated the testing docs PTAL Thanks!

vojtechjelinek

LGTM from code owner's perspective. Thanks!

vojtechjelinek

LGTM from code owner's perspective. Thanks!

nithusha21

LGTM as a codeowner.

System Administrator and others added 2 commits June 16, 2019 01:05

initial commit

efd2d4f

test commit

1d0c114

NishealJ added WIP labels Jun 15, 2019

NishealJ requested review from apb7, nithusha21, seanlip and vojtechjelinek as code owners June 15, 2019 22:10

NishealJ self-assigned this Jun 15, 2019

NishealJ added 2 commits June 16, 2019 02:23

Fix backend tests

d93dc3d

Fix lints

2185c32

NishealJ removed PR: don't merge - UNTESTED labels Jun 16, 2019

apb7 suggested changes Jun 16, 2019

View reviewed changes

apb7 suggested changes Jun 17, 2019

View reviewed changes

apb7 added the CHANGELOG: Code health label Jun 17, 2019

Revert fsevent

1b7e47f

apb7 reviewed Jun 18, 2019

View reviewed changes

Removed unwanted str

6e1ec76

apb7 approved these changes Jun 19, 2019

View reviewed changes

apb7 assigned vojtechjelinek, seanlip and nithusha21 Jun 19, 2019

seanlip removed their assignment Jun 19, 2019

Revert package-lock.json

f2fe911

vojtechjelinek approved these changes Jun 19, 2019

View reviewed changes

nithusha21 approved these changes Jun 20, 2019

View reviewed changes

nithusha21 unassigned vojtechjelinek and nithusha21 Jun 20, 2019

seanlip approved these changes Jun 20, 2019

View reviewed changes

seanlip merged commit 995b667 into oppia:develop Jun 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrades isort webtest and pylint quotes #6938

Upgrades isort webtest and pylint quotes #6938

NishealJ commented Jun 15, 2019 •

edited

Loading

codecov bot commented Jun 16, 2019

codecov bot commented Jun 16, 2019 •

edited

Loading

NishealJ commented Jun 16, 2019

apb7 left a comment

apb7 Jun 16, 2019

NishealJ Jun 17, 2019

apb7 Jun 17, 2019

NishealJ Jun 17, 2019

apb7 Jun 18, 2019

NishealJ Jun 18, 2019

NishealJ Jun 18, 2019

apb7 Jun 16, 2019

NishealJ Jun 17, 2019

apb7 Jun 17, 2019

apb7 Jun 17, 2019

NishealJ Jun 17, 2019

apb7 Jun 18, 2019

NishealJ Jun 18, 2019

apb7 commented Jun 17, 2019

NishealJ commented Jun 18, 2019

apb7 left a comment

apb7 Jun 18, 2019

apb7 Jun 18, 2019

apb7 left a comment

seanlip commented Jun 19, 2019

NishealJ commented Jun 19, 2019 •

edited

Loading

vojtechjelinek left a comment

vojtechjelinek left a comment

nithusha21 left a comment

Upgrades isort webtest and pylint quotes #6938

Upgrades isort webtest and pylint quotes #6938

Conversation

NishealJ commented Jun 15, 2019 • edited Loading

Explanation

Checklist

codecov bot commented Jun 16, 2019

Codecov Report

codecov bot commented Jun 16, 2019 • edited Loading

Codecov Report

NishealJ commented Jun 16, 2019

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 commented Jun 17, 2019

NishealJ commented Jun 18, 2019

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

seanlip commented Jun 19, 2019

NishealJ commented Jun 19, 2019 • edited Loading

vojtechjelinek left a comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

nithusha21 left a comment

Choose a reason for hiding this comment

NishealJ commented Jun 15, 2019 •

edited

Loading

codecov bot commented Jun 16, 2019 •

edited

Loading

NishealJ commented Jun 19, 2019 •

edited

Loading