Fix #4829: Reduce code redundancy in backend testing #5838

lilithxxx · 2018-11-08T15:16:23Z

Explanation

Fixes #4829

A new function is introduced get_html to get a HTML response analogous to get_json

Replaces instances of self.testapp.get with either self.get_json or self.get_html to reduce code redundancy.

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 follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- 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.

codecov-io · 2018-11-08T16:03:12Z

Codecov Report

Merging #5838 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #5838   +/-   ##
========================================
  Coverage    45.21%   45.21%           
========================================
  Files          523      523           
  Lines        30722    30722           
  Branches      4605     4605           
========================================
  Hits         13892    13892           
  Misses       16830    16830

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 718d4d0...08b1223. Read the comment docs.

oppiabot · 2018-11-18T16:44:52Z

Hi @lilithxxx, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

lilithxxx · 2018-11-19T09:42:07Z

Hi @seanlip @apb7 review please :)

seanlip

This looks great in general, thanks @lilithxxx! Left some inline comments.

Also, does this remove all occurrences of self.testapp.get in the codebase?

seanlip · 2018-11-19T10:00:22Z

core/controllers/admin_test.py

@@ -38,26 +38,28 @@ def setUp(self):
    def test_admin_page_rights(self):
        """Test access rights to the admin page."""

-        response = self.testapp.get('/admin')
+        response = self.get_html('/admin', expect_errors=True,


Here and elsewhere, status codes beginning with 2 or 3 should not be considered errors.

If that's the case then should'nt the condition here change to assert both 2xx and 3xx codes?

Yes, I think so. Thanks for catching that!

seanlip · 2018-11-19T10:00:57Z

core/controllers/admin_test.py

@@ -38,26 +38,28 @@ def setUp(self):
    def test_admin_page_rights(self):
        """Test access rights to the admin page."""

-        response = self.testapp.get('/admin')
+        response = self.get_html('/admin', expect_errors=True,


Here and below, prefer breaking after '(' if contents of parens span multiple lines.

seanlip · 2018-11-19T10:03:04Z

core/tests/test_utils.py

+        # https://github.com/Pylons/webtest/blob/
+        # bf77326420b628c9ea5431432c7e171f88c5d874/webtest/app.py#L1119 .
+        self.assertEqual(html_response.status_int, expected_status_int)
+        if not expect_errors:


expect_errors is only for status checks starting with 4 or 5.

Note that you should also check the converse, i.e. that if expect_errors is True then the status_int starts with 4 or 5.

lilithxxx · 2018-11-23T19:03:26Z

@seanlip this does not remove all occurrences of self.testapp.get. Some functions are having issues:

topic_editor_test -> This json object does not have attribute status_code
editor_test -> This has attribute text/plain.
resources_test -> This has attribute image/png.

There are a few other functions with similar problems(some has attribute audio etc)

seanlip · 2018-11-24T00:15:26Z

Thanks @lilithxxx. I don't understand the first one, I don't see 'status_code' in the response? (And in any case I don't understand why that behaves differently from similar json responses.)

For the content-type attrs maybe add an extra arg in the test_utils function to overwrite the default check?

I think if we are going to get rid of self.testapp.get() we should do it properly (i.e. completely), so please don't hesitate to ask me if there are any other cases which you'd like me to suggest a solution for.

Thanks!

lilithxxx · 2018-11-26T15:59:46Z

Hi @seanlip. There's another conflict in base.py -> Here the error codes can be 2xx, 3xx, or 4xx so the assertion fails in get_json where only a particular status code is checked.

topic_editor_test -> This json object does not have attribute status_code

There is one more error like this in collection_editor_test. What should I do about them?

seanlip · 2018-11-26T20:57:02Z

For the second one, since you're making individual requests, can't you pass the expected status code in with each request?

For the first one, you have a couple of options -- either generalize the method signature of get_json to accept a list of allowed expected status codes, or create a new method in test_utils that is similar to get_json but accepts a list of allowed codes instead of just a single one. The second might be better in order to keep the default get_json() simple.

oppiabot · 2018-11-28T07:34:07Z

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

lilithxxx · 2018-12-01T19:31:15Z

@seanlip The get_json method returns the body of the response. You can see that here. So we can't check status codes with this function as it doesn't have any attribute like that. I was thinking we can have a function called get_response and another function get_json that just loads the response obtained from the former method(get_response). The method get_response can be used for all content-type like html, image, audio, etc and thus we don't need get_html, we just can put content-type attr as parameter which will check for all cases. What do you think about this?

seanlip · 2018-12-01T19:43:53Z

The get_json method returns the body of the response. You can see that here. So we can't check status codes with this function as it doesn't have any attribute like that.

I looked at the code for get_json(). It is doing a status verification check: https://github.com/oppia/oppia/blob/develop/core/tests/test_utils.py#L512

So, not sure what you mean by "we can't check status codes with this function". I'm talking about modifying the logic of get_json itself, or writing a function that is similar to get_json but that accepts a list of expected status codes instead of just one.

I feel like I might be missing something in your response, though, since the original discussion wasn't about generalizing to html/image/audio/etc.

lilithxxx · 2018-12-01T19:52:59Z

I think I was not very clear, pardon me. Yes it does a status verification check but that is on the response object which we get from get.testapp but what the function ultimately returns is the body of response ( after calling _parse_json_response) and what we get from get_json is not a response object but a json dict which does not have a status attribute. I was not talking about the function that accepts a list of status codes( I understand that fully and I will implement it) but I was talking about the problem with the responses not having status_code attributes

seanlip · 2018-12-01T20:04:29Z

Thanks for explaining. I think the problem you're encountering is one I addressed in my previous comment, though?

I.e. you wouldn't change get_json to return the status code. You'd pass the expected status code(s) into get_json or similar and get_json (or the other function) would check those codes internally.

lilithxxx · 2018-12-02T17:53:49Z

@seanlip I renamed the function get_html to get_response as it just returns the get response after doing some tests. The other file formats are also called with this function. PTAL

seanlip · 2018-12-02T18:44:41Z

core/tests/test_utils.py

-            html_response.content_type, 'text/html')
-
-        return html_response
+        if not can_have_multiple_responses:


I think can_have_multiple_responses is a bit of a misleading variable name. I think what you're trying to convey is more like "can be an error or not", i.e. something that contradicts expect_errors being a boolean.

So I think maybe it's worth leaving this method signature as is (I am fine with changing get_html to get_response) and creating a separate method along the lines of get_response_without_checking_for_errors(). Then have the base.py test that runs through all the URLs use that latter method instead. In fact, maybe this method here could use that method too.

lilithxxx · 2018-12-03T20:38:45Z

@seanlip made changes. PTAL
(Also I did what you asked and merged develop into my branch and solved the conflicts locally but still it was unable to push so i had to force it)

oppiabot · 2018-12-16T18:03:04Z

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

apb7 · 2018-12-19T07:58:25Z

Hi @lilithxxx, any updates on this? Thanks!

lilithxxx · 2018-12-19T10:29:59Z

I will get back to it tomorrow as I am travelling currently. Thanks

lilithxxx · 2018-12-20T20:20:50Z

Hi @seanlip @apb7 made the changes. PTAL

oppiabot · 2018-12-22T04:09:10Z

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

oppiabot · 2019-01-01T04:35:45Z

Hi @lilithxxx, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

lilithxxx · 2019-01-04T18:08:32Z

Hi @seanlip PTAL at this as this PR has been marked stale.Thanks!

seanlip · 2019-01-04T18:12:41Z

Sorry @lilithxxx for the delay. Could you please resolve the merge conflicts first?

lilithxxx · 2019-01-04T18:25:49Z

@seanlip done!

seanlip

Thanks @lilithxxx -- did another pass, PTAL.

seanlip · 2019-01-04T19:28:44Z

core/tests/test_utils.py

+
+        Args:
+            url: str. The URL to fetch the response.
+


Drop line breaks between successive arg descriptions (please follow existing codebase style). In addition, for line descriptions that span more than one line, indent the continuation line by 4 spaces (again, follow the existing style).

core/tests/test_utils.py

seanlip · 2019-01-04T19:30:41Z

core/tests/test_utils.py

+        """
+        have_errors = False
+        if expected_status_int >= 400 and (
+                expected_status_int < 600):


Don't think you need this second part -- just expected_status_int >= 400 should suffice.

seanlip · 2019-01-04T19:31:14Z

core/tests/test_utils.py

-                     expected_status_int=200,
-                     expected_content_type='text/html'):
-        """Get a response, transformed to a Python object."""
+    def get_response(


Make private (by prepending underscore)? That ensures it doesn't get used elsewhere -- only by get_html_response etc.

get_response is being used by other functions where the content-type is different than html or json -- for example png or text/plain.

Ah, I see. Then, I suggest: make get_response private, and define a new get_custom_response that first checks that the content type is not html/json, and that then calls _get_response(). External callers should use get_custom_response(). I think that would be a cleaner API.

seanlip · 2019-01-04T19:31:50Z

core/tests/test_utils.py

+        Args:
+            url: str. The URL to fetch the response.
+
+            params: str or dict. A query string, or a dictionary that will be


See above notes on how to write docstrings and on the idea of making the expected usage of params more explicit.

seanlip · 2019-01-04T19:32:18Z

core/tests/test_utils.py

+            expected_status_int_list: list(int). A list of integer status
+            code to expect.
+
+            params: str or dict. A query string, or a dictionary that will be


seanlip · 2019-01-04T19:33:42Z

core/tests/test_utils.py

        """Get a JSON response, transformed to a Python object."""
+        have_errors = False


Var name should probably be expect_errors. Currently you are trying to have this variable state whether the test expects an error after the response comes back. It doesn't say whether the response actually has an error or not.

Similar comments might apply elsewhere; please check.

lilithxxx · 2019-01-06T15:40:10Z

Hi @seanlip I made some changes, can you PTAL?

seanlip · 2019-01-06T15:39:34Z

core/controllers/reader_test.py

-        csrf_token = self.get_csrf_token_from_response(response)
-        summaries = self.get_json(
-            recommendations_url, params=csrf_token)['summaries']
+        self.get_html_response('/explore/%s' % exploration_id)


Why do you need this call?

For checking status code and content-type?

Hm. The original purpose of the call was to get the response so you can get the CSRF token so that you can make the subsequent call.

If you don't need the CSRF token then do you actually need this?

Hmm I think you are right we don't need this now.

seanlip · 2019-01-06T15:40:14Z

core/tests/test_utils.py

-        if expected_status_int >= 400 and (
-                expected_status_int < 600):
-            have_errors = True
+        if params:


if params is not None

seanlip · 2019-01-06T15:40:54Z

core/controllers/editor_test.py

-        response = self.get_response(download_url,
-                                     expected_content_type='text/plain')
+        response = self.get_custom_response(
+            download_url, expected_content_type='text/plain')


You don't need to specify the argname for expected_content_type, since it has no default value.

The linter should catch this. Maybe file a bug?

Please address my last paragraph, either with a link to the issue filed or some explanation for why the linter didn't catch this.

So sorry -- I was looking into this and I think pylint reports when we don't use the argname in case of a default value parameter but not the other way around. @apb7 Can you confirm this? So I think this is to fall under the category of a new feature. I am opening an issue

Opened #6067

@seanlip @lilithxxx, I can confirm this. Thanks for filling the issue, @lilithxxx.

seanlip · 2019-01-06T15:41:17Z

core/controllers/resources_test.py

-                                     expected_content_type='image/png')
+        response = self.get_custom_response(
+            self._get_image_url('0', filename),
+            expected_content_type='image/png')


lilithxxx · 2019-01-06T16:24:23Z

@seanlip PTAL

seanlip

LGTM. Thanks @lilithxxx!

Replaced testapp.get with appropriate functions

3c32fb5

lilithxxx requested review from apb7 and seanlip as code owners November 8, 2018 15:16

oppiabot bot added the stale label Nov 18, 2018

oppiabot bot removed the stale label Nov 19, 2018

seanlip requested changes Nov 19, 2018

View reviewed changes

Add test cases

846a5d8

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Nov 28, 2018

Function get_response

982d983

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Dec 2, 2018

seanlip reviewed Dec 2, 2018

View reviewed changes

Rishav Chakraborty added 4 commits December 4, 2018 00:13

Function get_response_without_checking_for_errors

ba2eafc

Resolved conflicts

a04ffb5

Fixed Errors

3b12b1e

Fixed lint checks

014d687

lilithxxx force-pushed the remove_testapp branch from 305d706 to 014d687 Compare December 3, 2018 20:28

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Dec 16, 2018

Rishav Chakraborty added 4 commits December 21, 2018 00:58

removed further redundancy

132f541

Resolved conflicts

df8412f

Lint checks

ab08985

Lint checks

1288bd5

oppiabot bot added PR: don't merge - HAS MERGE CONFLICTS and removed PR: don't merge - HAS MERGE CONFLICTS labels Dec 20, 2018

oppiabot bot added the stale label Jan 1, 2019

oppiabot bot removed the stale label Jan 4, 2019

Merge develop

acd0c1a

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jan 4, 2019

seanlip reviewed Jan 4, 2019

View reviewed changes

made params only dict type

dddb91c

seanlip reviewed Jan 6, 2019

View reviewed changes

Minor changes

08b1223

seanlip approved these changes Jan 6, 2019

View reviewed changes

seanlip added the PR: LGTM label Jan 6, 2019

kevinlee12 merged commit 35c2f13 into oppia:develop Jan 6, 2019

lilithxxx deleted the remove_testapp branch January 7, 2019 06:40

		"""Get a JSON response, transformed to a Python object."""
		have_errors = False

Fix #4829: Reduce code redundancy in backend testing #5838

Fix #4829: Reduce code redundancy in backend testing #5838

Conversation

lilithxxx commented Nov 8, 2018 • edited Loading

Explanation

Checklist

codecov-io commented Nov 8, 2018 • edited Loading

Codecov Report

oppiabot bot commented Nov 18, 2018

lilithxxx commented Nov 19, 2018

seanlip 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

lilithxxx commented Nov 23, 2018

seanlip commented Nov 24, 2018 • edited Loading

lilithxxx commented Nov 26, 2018

seanlip commented Nov 26, 2018

oppiabot bot commented Nov 28, 2018

lilithxxx commented Dec 1, 2018

seanlip commented Dec 1, 2018

lilithxxx commented Dec 1, 2018

seanlip commented Dec 1, 2018

lilithxxx commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilithxxx commented Dec 3, 2018

oppiabot bot commented Dec 16, 2018

apb7 commented Dec 19, 2018

lilithxxx commented Dec 19, 2018

lilithxxx commented Dec 20, 2018

oppiabot bot commented Dec 22, 2018

oppiabot bot commented Jan 1, 2019

lilithxxx commented Jan 4, 2019

seanlip commented Jan 4, 2019

lilithxxx commented Jan 4, 2019

seanlip 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

lilithxxx commented Jan 6, 2019

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

lilithxxx Jan 6, 2019 • edited Loading

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

lilithxxx commented Jan 6, 2019

seanlip left a comment

Choose a reason for hiding this comment

lilithxxx commented Nov 8, 2018 •

edited

Loading

codecov-io commented Nov 8, 2018 •

edited

Loading

seanlip commented Nov 24, 2018 •

edited

Loading

lilithxxx Jan 6, 2019 •

edited

Loading