Fix part of #6550: Write tests for controllers/editor #6849

lilithxxx · 2019-06-05T16:29:55Z

Explanation

Fixes part of #6550: Write tests for controllers/editor

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.

lilithxxx · 2019-06-05T18:21:44Z

core/controllers/editor.py

@@ -468,21 +453,6 @@ def post(self, unused_exploration_id):
        })


-class ExplorationResourcesHandler(EditorHandler):


I don't think this handler is used in the front-end. Can anyone confirm this?

I agree, we can drop it and drop the corresponding stanza in main.py. Good catch!

vinitamurthi · 2019-06-05T20:08:16Z

practice_sessions.py looks fine to me, LGTM on that! Just one question though, is this test passing?

lilithxxx · 2019-06-05T20:12:06Z

@vinitamurthi yes they do. I removed those two conditions as the existence of topics are checked in their respective decorators, hence we don't need to double check it in the controller

vinitamurthi · 2019-06-05T20:14:33Z

Okay that's great, thanks @lilithxxx !

kevinlee12

lgtm! (from code owner's perspective)

oppiabot · 2019-06-09T19:07:47Z

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!

vojtechjelinek

LGTM from code owner's perspective. Thanks!

seanlip

Looks pretty good, thanks @lilithxxx! Took a pass.

seanlip · 2019-06-10T01:51:51Z

core/controllers/acl_decorators_test.py

+        self.login(self.OWNER_EMAIL)
+        with self.swap(self, 'testapp', self.mock_testapp):
+            self.get_json(
+                '/mock/invalid_collection_id', expected_status_int=404)


Use a better name than /mock (might be happening in #6782 already though).

seanlip · 2019-06-10T01:54:26Z

core/controllers/editor.py

@@ -271,11 +263,7 @@ def put(self, exploration_id):

        elif make_community_owned:
            exploration = exp_services.get_exploration_by_id(exploration_id)
-            try:
-                exploration.validate(strict=True)
-            except utils.ValidationError as e:


This needs to stay, don't assume old production data is necessarily valid.

In the line above, we are using exploration.validate(strict=True). Strict means that it will raise an exception from the validate function and hence this line becomes unreachable.

Why is it unreachable? The whole point of try/except is to catch an exception, and if a ValidationError is raised then the except clause will be triggered.

seanlip · 2019-06-10T01:55:11Z

core/controllers/editor.py

@@ -214,11 +208,9 @@ def put(self, exploration_id):
        except utils.ValidationError as e:
            raise self.InvalidInputException(e)

-        try:


Why delete?

get_user_exploration_data()'s failure cases are handled by its decorator, i.e can_edit_exploration

I don't see a reference to the ExplorationUserData model there. What if it's just not present for some reason? Should still catch this.

But how can I create an exception here? It won't throw an exception if ExplorationUserData model is None -- it will work fine. I think the possible reasons for raising an exception is covered by its decorator.

There's a line in that function that does:

exp_user_data = user_models.ExplorationUserDataModel.get( user_id, exploration_id)

What happens if that model instance doesn't exist?

It returns None and the code below it works fine even if the model is None

Ah I see, thanks for explaining. Yes, what you did sounds fine, then.

seanlip · 2019-06-10T01:57:33Z

core/controllers/editor.py

@@ -602,7 +563,7 @@ def get(self, exp_id):
        exp_issues = stats_services.get_exp_issues(exp_id, exp_version)
        if exp_issues is None:
            raise self.PageNotFoundException(
-                'Invalid exploration ID %s' % (exp_id))
+                'Invalid exploration version %s' % (exp_version))


Maybe a more specific error message might be better -- Invalid version %s for exploration ID %s

seanlip · 2019-06-10T01:58:45Z

core/controllers/editor_test.py

            (exp_id, feconf.OUTPUT_FORMAT_JSON))
        response = self.get_json(download_url)

        # Check downloaded dict.
        self.assertEqual(self.SAMPLE_JSON_CONTENT, response)

-        # Check downloading a specific version.


Why delete all this? Please don't delete code/tests without explanation. (Ditto above -- I understand those that were deleted because the decorator took care of the check, but everything else needs to be justified.)

Oops a mistake. Reverted it!

seanlip · 2019-06-10T02:03:11Z

core/controllers/editor_test.py

+                '/createhandler/state_rules_stats/%s/invalid_state_name'
+                % (exp_id), expected_status_int=404)
+
+        self.assertEqual(len(observed_log_messages), 2)


This and the following lines read oddly. Why are we only checking one of these?

seanlip · 2019-06-10T02:05:57Z

core/controllers/editor_test.py

+        response = self.get_html_response('/create/%s' % exp_id)
+        csrf_token = self.get_csrf_token_from_response(response)
+        exploration = exp_services.get_exploration_by_id(exp_id)
+        with self.assertRaisesRegexp(


Shouldn't this (and the following) have something that uses expected_status_int and response['error'], like all the other tests? We don't want to surface 500s to the user.

seanlip · 2019-06-10T02:06:21Z

core/controllers/editor_test.py

+        response = self.get_html_response(
+            '%s/%s' % (feconf.EDITOR_URL_PREFIX, exp_id))
+        csrf_token = self.get_csrf_token_from_response(response)
+        with self.assertRaisesRegexp(Exception, 'Invalid message type.'):


seanlip · 2019-06-10T02:06:33Z

core/controllers/editor_test.py

@@ -1475,6 +1874,16 @@ def setUp(self):
            }]
        )

+    def test_cannot_fetch_issues_with_invalid_version(self):
+        self.get_json(
+            '/issuesdatahandler/%s' % (self.EXP_ID),


Nit: No need parens around self.EXP_ID

seanlip · 2019-06-10T02:06:51Z

core/controllers/library.py

@@ -191,7 +191,7 @@ def get(self):
                header_i18n_id = feconf.LIBRARY_CATEGORY_TOP_RATED_EXPLORATIONS

        else:
-            return self.PageNotFoundException
+            raise self.PageNotFoundException


Good catch!

nithusha21

LGTM for changes in main.py (as a codeowner)

codecov · 2019-06-10T18:40:54Z

Codecov Report

Merging #6849 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #6849      +/-   ##
===========================================
+ Coverage    95.12%   95.18%   +0.05%     
===========================================
  Files          371      371              
  Lines        50429    50839     +410     
===========================================
+ Hits         47973    48392     +419     
+ Misses        2456     2447       -9

Impacted Files	Coverage Δ
main.py	`87.37% <ø> (ø)`	⬆️
core/controllers/practice_sessions.py	`100% <ø> (+5.71%)`	⬆️
core/controllers/editor_test.py	`100% <100%> (+0.42%)`	⬆️
core/controllers/resources_test.py	`100% <100%> (ø)`	⬆️
core/controllers/acl_decorators_test.py	`100% <100%> (ø)`	⬆️
core/controllers/collection_viewer.py	`100% <100%> (ø)`	⬆️
core/controllers/acl_decorators.py	`89.51% <100%> (+0.95%)`	⬆️
core/controllers/library.py	`100% <100%> (ø)`	⬆️
core/controllers/editor.py	`99.39% <100%> (+16.75%)`	⬆️
core/platform/image/gae_image_services.py	`65.21% <0%> (-34.79%)`	⬇️
... and 47 more

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 a4efcdb...e645367. Read the comment docs.

codecov · 2019-06-10T18:40:57Z

Codecov Report

Merging #6849 into develop will increase coverage by 0.17%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #6849      +/-   ##
===========================================
+ Coverage     95.5%   95.67%   +0.17%     
===========================================
  Files          371      371              
  Lines        51058    51285     +227     
===========================================
+ Hits         48762    49068     +306     
+ Misses        2296     2217      -79

Impacted Files	Coverage Δ
main.py	`87.37% <ø> (ø)`	⬆️
core/controllers/practice_sessions.py	`100% <ø> (+5.71%)`	⬆️
core/controllers/editor_test.py	`100% <100%> (+0.41%)`	⬆️
core/controllers/resources_test.py	`100% <100%> (ø)`	⬆️
core/controllers/acl_decorators_test.py	`100% <100%> (ø)`	⬆️
core/controllers/acl_decorators.py	`98.66% <100%> (-0.32%)`	⬇️
core/controllers/library.py	`100% <100%> (ø)`	⬆️
core/controllers/editor.py	`100% <100%> (+17.81%)`	⬆️
core/domain/rights_manager.py	`91.26% <0%> (-0.27%)`	⬇️
... and 3 more

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 98bf4fb...3fb9f3c. Read the comment docs.

lilithxxx · 2019-06-10T18:42:16Z

@seanlip PTAL!

seanlip · 2019-06-10T19:35:48Z

core/controllers/editor_test.py

@@ -762,39 +779,6 @@ def test_record_user_saw_tutorial(self):
        self.logout()


-class StateAnswerStatisticsHandlerTests(test_utils.GenericTestBase):


Why is this deleted?

Consider preemptively justifying any deletions in a comment to the review thread, otherwise reviewers will just ask that question during reviews and that will slow things down.

StateAnswerStatisticsHandlerTests was added in PR #6679. That's why I deleted this duplicate test class

seanlip · 2019-06-10T19:37:24Z

core/controllers/editor_test.py

@@ -872,6 +855,10 @@ def _mock_logging_function(msg, *args):
            observed_log_messages[0],
            'Could not find state: invalid_state_name')

+        self.assertEqual(


You might as well replace these three assertions with a single self.assertEqual(observed_log_messages, ...)

seanlip · 2019-06-10T19:39:06Z

Please also note #6849 (comment) and #6849 (comment) above.

lilithxxx · 2019-06-10T20:56:02Z

@seanlip PTAL!

seanlip

Thanks @lilithxxx! Only one comment left.

seanlip · 2019-06-11T07:24:46Z

core/controllers/editor_test.py

@@ -1467,6 +1466,40 @@ def test_transfering_ownership_to_the_community(self):

        self.logout()

+    def test_cannot_transfer_ownership_to_the_community_with_invalid_lang_code(


"lang code" is a bit specific for what you're actually trying to validate. A developer will read this and wonder "what's so special about the lang code in particular?"

Suggest changing the test name to test_cannot_transfer_ownership_of_invalid_exp_to_the_community(), and adding the following lines in a separate "block" after line 1488:

# The exploration is now invalid. {{ADD CODE HERE: create the domain object from the model and check that exploration.validate() raises an error.}}

This makes it clearer that what you are really testing is "invalid explorations" and that the language code is just a specific case of that.

Hmm I did not understand you completely. I have added a comment to explain that the exploration is now invalid. Is this what you wanted or do I need to add something else?

I actually meant using get_exploration_from_model to create the corresponding domain object and asserting a validation failure. But I think what you've done works too (in terms of comprehensibility), so let's go with that. Thanks!

seanlip · 2019-06-12T03:41:47Z

@DubeySandeep PTAL as codeowner. Thanks!

lilithxxx · 2019-06-13T07:28:59Z

@DubeySandeep PTAL

nithusha21 · 2019-06-17T09:33:57Z

/cc @aks681 just flagging this... Needs to be cherrypicked. Thanks!

Rishav Chakraborty added 5 commits June 3, 2019 01:18

Merge stash

ab8d5a2

Write tests for controllers/editor

b317870

Merge branch 'develop' into new_editor_test

8c1b30b

package-lock

73a5688

Fix lint

d417645

lilithxxx requested review from DubeySandeep, kevinlee12, nithusha21, seanlip, vinitamurthi and vojtechjelinek as code owners June 5, 2019 16:29

lilithxxx added the CHANGELOG: Testing coverage label Jun 5, 2019

Fix tests

8df5a37

lilithxxx commented Jun 5, 2019

View reviewed changes

vinitamurthi approved these changes Jun 5, 2019

View reviewed changes

kevinlee12 approved these changes Jun 7, 2019

View reviewed changes

brianrodri self-requested a review June 9, 2019 18:48

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jun 9, 2019

brianrodri assigned vojtechjelinek, seanlip, nithusha21 and DubeySandeep Jun 9, 2019

vojtechjelinek approved these changes Jun 9, 2019

View reviewed changes

brianrodri removed their request for review June 9, 2019 20:25

seanlip requested changes Jun 10, 2019

View reviewed changes

nithusha21 approved these changes Jun 10, 2019

View reviewed changes

nithusha21 removed their assignment Jun 10, 2019

Rishav Chakraborty added 2 commits June 10, 2019 23:29

Merge develop

e406e09

Addressed comments

e645367

lilithxxx unassigned vojtechjelinek Jun 10, 2019

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

seanlip requested changes Jun 10, 2019

View reviewed changes

Addressed comments

b74388c

seanlip requested changes Jun 11, 2019

View reviewed changes

Rishav Chakraborty added 2 commits June 11, 2019 15:38

Merge branch 'develop' into new_editor_test

c5b6344

Added comment

8c31ca0

seanlip approved these changes Jun 12, 2019

View reviewed changes

seanlip removed their assignment Jun 12, 2019

Merge develop

3fb9f3c

DubeySandeep approved these changes Jun 13, 2019

View reviewed changes

DubeySandeep merged commit 8f04ce2 into oppia:develop Jun 13, 2019

lilithxxx deleted the new_editor_test branch June 13, 2019 17:50

lilithxxx mentioned this pull request Jun 15, 2019

The add roles tab in the exploration settings tab raises 500 error when an unknown id is provided #6932

Closed

nithusha21 added the PR: for current release label Jun 17, 2019

aks681 added PR: released and removed PR: for current release labels Jun 19, 2019

aks681 removed the PR: released label Jul 4, 2019

		@@ -468,21 +453,6 @@ def post(self, unused_exploration_id):
		})


		class ExplorationResourcesHandler(EditorHandler):

		@@ -762,39 +779,6 @@ def test_record_user_saw_tutorial(self):
		self.logout()


		class StateAnswerStatisticsHandlerTests(test_utils.GenericTestBase):

		@@ -1467,6 +1466,40 @@ def test_transfering_ownership_to_the_community(self):

		self.logout()

		def test_cannot_transfer_ownership_to_the_community_with_invalid_lang_code(

Fix part of #6550: Write tests for controllers/editor #6849

Fix part of #6550: Write tests for controllers/editor #6849

Conversation

lilithxxx commented Jun 5, 2019 • edited Loading

Explanation

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinitamurthi commented Jun 5, 2019

lilithxxx commented Jun 5, 2019

vinitamurthi commented Jun 5, 2019

kevinlee12 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Jun 9, 2019

vojtechjelinek left a comment

Choose a reason for hiding this comment

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

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

nithusha21 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2019

Codecov Report

codecov bot commented Jun 10, 2019 • edited Loading

Codecov Report

lilithxxx commented Jun 10, 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

seanlip commented Jun 10, 2019

lilithxxx commented Jun 10, 2019

seanlip left a comment

Choose a reason for hiding this comment

seanlip Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jun 12, 2019

lilithxxx commented Jun 13, 2019

nithusha21 commented Jun 17, 2019

lilithxxx commented Jun 5, 2019 •

edited

Loading

codecov bot commented Jun 10, 2019 •

edited

Loading

seanlip Jun 11, 2019 •

edited

Loading