Fix part of #5134: Increase test coverage for core.controller.skill_editor #5686

ghost · 2018-09-23T04:35:15Z

This PR is mainly concerned with increasing test coverage for core.controller.skill_editor as part of #5134 (from 60% to ~~88%~~ ~~98%~~ 100%)

~~For the remaining uncovered lines, I don't know how to make them covered without having a way to mock the returned results of functions in skill_services or user_services.~~

I also refactor some code in the test file.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
[-] The PR explanation includes the words "Fixes #bugnum: ...".
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.

seanlip · 2018-09-23T05:15:53Z

Hi @Xuanluong, to some extent the tests in core.controllers.skill_editor are sometimes integration tests of the whole backend, so I think you can set things up without mocking by returning the relevant things from skill_services or user_services?

Alternatively, you could use self.swap() to mock results -- it's defined in core.tests.test_utils and there are several examples of it in the codebase. You can do something like "with self.swap(skill_services, 'name_of_function_you_want_to_swap', desired_return_value)" and put the relevant code underneath that header.

Hope that helps!

ghost · 2018-09-23T05:20:42Z

@seanlip thanks for the tip. I'll try it.

codecov-io · 2018-09-23T05:42:07Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #5686   +/-   ##
========================================
  Coverage    45.76%   45.76%           
========================================
  Files          513      513           
  Lines        29919    29919           
  Branches      4522     4522           
========================================
  Hits         13691    13691           
  Misses       16228    16228

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 7ff68c9...534b2dd. Read the comment docs.

…ollers.skill_editor

seanlip · 2018-09-24T06:59:48Z

Hi @Xuanluong -- 98% is tantalizingly close! Just to check, are you having trouble with the other 2%? If you have specific questions, let us know :)

ghost · 2018-09-27T00:49:15Z

@seanlip There're 2 lines of code left to be covered:
One of them is this:

oppia/core/controllers/skill_editor.py

Line 185 in a325d54

raise self.PageNotFoundException

This line looks like dead code to me because it can only be reached if skill_id is falsy (its value is False, None, empty string, etc.). Any falsy value would make skill_domain.Skill.require_valid_skill_id(skill_id) on the previous line raise an exception.

So I don't think this line can be covered.

I'm working on the other line but haven't had time in the last few days.

seanlip · 2018-09-27T02:16:31Z

Thanks @Xuanluong! I agree with you that the line you mentioned is dead code and can probably be removed. @aks681 can you confirm this please?

seanlip · 2018-09-27T02:18:42Z

core/controllers/skill_editor_test.py

+        self.logout()
+
+
+def _get_skill_by_id(*_args, **_kwargs):


Don't declare these globally -- make them private methods in the class and maybe call them _mock_get_skill_by_id or similar.

Also typically I don't think there are underscores before *args and **kwargs. That said, probably better to use the actual args that match the mocked method. You can prefix each of them by unused_ to silence lint errors (assuming they are actually unused).

Prefixing unused variables only work for positional arguments. For keyword arguments, if we prefix them with unused, the signature of the mock method will no long match that of the real method. I guess we have to use something like **unused_kwargs.

I have made the methods private and prefix unused for some arguments in the mock methods. Please take a look.

…ollers.skill_editor

ghost · 2018-09-27T03:30:57Z

core/controllers/skill_editor_test.py

+            self.assertEqual(json_resp['can_edit_skill_description'], False)
+        self.logout()
+
+    def test_skill_rights_handler_fails(self):


@seanlip This test aims to cover this line

oppia/core/controllers/skill_editor.py

Line 104 in a325d54

raise self.InvalidInputException(

I've spent quite a lot of time making the test cover that lines without success. Perhaps you can figure out what is missing in the setup?

@aks681 Can you help me with this issue?

I am not sure why that does not work, but can you try @seanlip's comments below about using an invalid skill_id directly and check if that works?
Also, to try troubleshooting, see the docstring for the swap function itself here:

oppia/core/tests/test_utils.py

Line 1100 in a325d54

def swap(self, obj, attr, newvalue):

Maybe that will help.

@seanlip @aks681
I want to follow up with Sean's suggestion for this test.

I tried this

model = skill_models.SkillRightsModel.get(self.skill_id) model.delete(self.admin_id, 'Deleting the model.')

before making the GET request. I also put debugging lines in skill_services.get_skill_rights and it did return None. However, the debugging line on skill_editor was not printed out on the screen and the line is still not covered.

One thing worth notice is the return error is still as expected (404). If the line of code that we're concerned with is not reached, where in the code that an exception was raised that lead to the 404 error?

I think there are two things going on here.

Firstly, your code might be hitting the decorator (see the comment I just left previously) since you haven't created a skill.

Secondly, the suggestion I have is actually to create a skill the normal way (using skill_services.save_new_skill) and then manually delete the rights model. I think that should (?) yield the desired result.

@seanlip I think this is dead code because the decorator code checks the same thing here

oppia/core/domain/acl_decorators.py

Line 1417 in f3cf204

skill_rights = skill_services.get_skill_rights(skill_id, strict=False)

Even if we follow your second suggestion, those lines of code in skill_editor.py would not be reached anyway.

Let's delete the dead code?

Yep! Definitely, let's delete any dead code.

seanlip

Thanks @Xuanluong! I took a full pass, PTAL. (@aks681 you might want to take a pass too.)

Re the question you had, I think that should be guarded against, although it would only arise in practice due to some error with writing data to the backend. I suggest creating a test that creates a skill, and then delete the rights model manually using functions in the domain or storage layer -- then pinging the controller.

Does that help?

seanlip · 2018-09-27T03:31:14Z

core/controllers/skill_editor_test.py

+    def _mock_get_skill_by_id(self, unused_skill_id, **unused_kwargs):
+        return None
+
+


Drop extra newline.

seanlip · 2018-09-27T03:35:16Z

core/controllers/skill_editor_test.py

+        self.get_json(self.url)
+        # Check GET returns JSON object with can_edit_skill_description set
+        # to False if the user is not allowed to edit the skill description.
+        def _get_all_actions(*_args):


_mock_get_all_actions, and make into class-level function (like in BaseSkillEditorControllerTest). Add short docstring explaining what it's doing conceptually.

Even better: perhaps just test the permissions directly (without a mock) for non-admin users.

seanlip · 2018-09-27T03:35:52Z

core/controllers/skill_editor_test.py

+            actions.remove(role_services.ACTION_EDIT_SKILL_DESCRIPTION)
+            return actions
+        with self.swap(role_services, 'get_all_actions', _get_all_actions):
+            json_resp = self.get_json(self.url)


Prefer json_response; try not to use abbreviations.

seanlip · 2018-09-27T03:36:33Z

core/controllers/skill_editor_test.py


-            # Check that admins can access the editable skill data.
+        # Check GET returns 404 when cannot get skill by id.


Typically, the controller tests are integration tests for the server (of a sort). So, instead of mocking here, maybe just do a GET for a skill ID which does not exist?

I just tried that. The test passed but the line below is not covered:

oppia/core/controllers/skill_editor.py

Line 66 in 8f3d11c

if skill is None:

I use skill_services.get_new_skill_id() to get a new skill ID. I don't call save_new_skill so get_skill_by_id should return None. However, it seems like the code never reaches the call to get_skill_by_id in this case. The reason I think so is because change the expected error to 500 so that I can add some debugging lines at several places in the code, e.g. in the body of the get method. None of those debugging lines. In addition, I got this error message:

WARNING:root:Invalid URL requested: http://localhost/skill_editor/F4ynhfByGQvn

This skill ID should pass the check of require_valid_skill_id, so I'm suspecting somewhere in the code we're checking if the skill exists even before the method handling the GET request is called.

Ah! I think you might have found even more dead code, which is great since it means we can remove it.

There's a decorator on the handler which checks the skill beforehand for validity:

https://github.com/oppia/oppia/blob/develop/core/controllers/skill_editor.py#L55

I think that might have a check for validity, so that this one is redundant?

So, do want to keep the mock now to make the coverage 100%?

Hm, not totally sure I understand the suggestion. Shouldn't we just delete the dead code instead?

I suggest removing the dead code in the other test because the decorator calls get_skill_rights so the check if skill_rights is None is already handled by the decorator.

In this thread we're discussing tests in which I mock the call to skill_services.get_skill_by_id.

In theory, if a skill id doesn't exist, both get_skill_rights (called by the decorator) and get_skill_by_id (called by the methods in skill_editor.py) will return None.

In practice, I don't know if the statement above will always hold true, or there might be cases in which the call in the decorator doesn't return None but the calls in skill_editor.py do, so I would prefer to keep these lines of code:

if skill_id is None: raise Exception ...

in skill_editor.py.

If we want to keep the lines of code above and want to have 100% coverage, we would have to mock get_skill_by_id.

So, my question is which option do we choose:
1/ Remove the if skill_id is None checks wherever in the code that get_skill_by_id is used (dead code).
2/ Keep the if skill_id is None checks and mock get_skill_by_id in the test to make the coverage 100%

@seanlip ?

Ah, so, I think I get it. You're thinking about the case where skill rights is not None, but the skill ID retrieval fails, right? Generally that should not happen, but theoretically it is a possibility since they are two different models.

So, how about doing the reverse of what I suggested for another test? I.e. create the skill the usual way (which causes both a skill model and a skill rights model to be created), then delete the skill model manually. Then you don't need to mock things, and you can still test the line you're referring to.

seanlip · 2018-09-27T03:39:25Z

core/controllers/skill_editor_test.py

+            return actions
+        with self.swap(role_services, 'get_all_actions', _get_all_actions):
+            json_resp = self.get_json(self.url)
+            self.assertEqual(json_resp['can_edit_skill_description'], False)


For this check to have meaning there should probably be a corresponding one where can_edit_skill_description is asserted to be True.

seanlip · 2018-09-27T03:40:54Z

core/controllers/skill_editor_test.py

@@ -96,47 +157,134 @@ def test_editable_skill_handler_put(self):
                'new_value': 'New Description'
            }]
        }
+
+


Drop extra newline.

seanlip · 2018-09-27T03:41:53Z

core/controllers/skill_editor_test.py

+            self.assertEqual(response.status_int, 404)
+        # Check GET returns 404 when cannot get skill by id.
+        feconf_swap = self.swap(feconf, 'ENABLE_NEW_STRUCTURES', True)
+        skill_services_swap = self.swap(


Same thing, maybe just use an invalid ID

Done.

Ditto.

seanlip · 2018-09-27T03:42:25Z

core/controllers/skill_editor_test.py

+            with self.swap(feconf, 'ENABLE_NEW_STRUCTURES', False):
+                self.put_json(self.url, self.put_payload, csrf_token=csrf_token,
+                              expect_errors=True, expected_status_int=404)
+            # Check PUT returns 404 when cannot get skill by id.


Done.

Ditto.

seanlip · 2018-09-27T03:43:36Z

core/controllers/skill_editor_test.py

+            # Check PUT returns 400 when an exception is raised updating the
+            # skill.
+            update_skill_swap = self.swap(
+                skill_services, 'update_skill', self._mock_update_skill)


Can we name _mock_update_skill in a way that conveys that it raises an exception? Perhaps self._raise_exception might be a good name (since then this line of code conveys more clearly that you're swapping update_skill with a function that raises an exception), but you might have other ideas too.

seanlip · 2018-09-27T03:45:11Z

core/controllers/skill_editor_test.py

+
+    def test_skill_publish_handler_fails(self):
+
+        def _publish_skill(*_args):


See note above re naming for clarity. Also maybe extract this into a class-level method.

aks681 · 2018-09-27T08:42:45Z

@seanlip Yeah, those 2 lines were there before the require_valid_skill_id function was defined and I think those can be removed now.

…ollers.skill_editor

…rn None

seanlip

LGTM. Thanks @Xuanluong!

Increase test coverage for core.controller.skill_editor

946d764

seanlip requested a review from aks681 September 23, 2018 05:13

seanlip assigned aks681 Sep 23, 2018

seanluong added 4 commits September 23, 2018 03:27

Restructure the text and use self.swap to cover more lines

9bc5e78

Merge branch 'develop' of github.com:oppia/oppia into 5134-core.contr…

e3bbd16

…ollers.skill_editor

Reorganize tests and increase coverage to 98%

b1f62a3

Merge branch 'develop' of github.com:oppia/oppia into 5134-core.contr…

a336738

…ollers.skill_editor

ghost changed the title ~~Fix part of #5134: Increase test coverage for core.controller.skill_editor~~ [DO NOT MERGE] Fix part of #5134: Increase test coverage for core.controller.skill_editor Sep 24, 2018

seanlip reviewed Sep 27, 2018

View reviewed changes

seanluong added 3 commits September 26, 2018 22:42

Merge branch 'develop' of github.com:oppia/oppia into 5134-core.contr…

3f77893

…ollers.skill_editor

Make mock method private and prefix unused arguments

7ce6c8c

Write test attempting to cover line 104 of skill_editor.py

cb57b1e

ghost commented Sep 27, 2018

View reviewed changes

seanlip requested changes Sep 27, 2018

View reviewed changes

seanluong added 5 commits September 27, 2018 21:36

Merge branch 'develop' of github.com:oppia/oppia into 5134-core.contr…

1df0342

…ollers.skill_editor

Address styling and naming comments.

9b97e47

Remove dead code.

13113ce

Merge branch 'develop' of github.com:oppia/oppia into 5134-core.contr…

fb8901b

…ollers.skill_editor

Remove dead code in SkillRightsHandler

3bca364

ghost changed the title ~~[DO NOT MERGE] Fix part of #5134: Increase test coverage for core.controller.skill_editor~~ Fix part of #5134: Increase test coverage for core.controller.skill_editor Sep 29, 2018

seanluong added 3 commits September 30, 2018 20:51

Merge branch 'develop' of github.com:oppia/oppia into 5134-core.contr…

5fc3922

…ollers.skill_editor

Delete skill model and key from memcache to make get_skill_by_id retu…

8e6e577

…rn None

Remove debugging print lines

534b2dd

seanlip approved these changes Oct 1, 2018

View reviewed changes

seanlip merged commit f6cba54 into oppia:develop Oct 1, 2018

pranavsid98 mentioned this pull request Oct 1, 2018

Finish writing remaining backend tests #5134

Closed

ghost deleted the 5134-core.controllers.skill_editor branch October 5, 2018 01:29

DubeySandeep mentioned this pull request Apr 3, 2019

Finish writing remaining backend tests #6550

Closed

		def _mock_get_skill_by_id(self, unused_skill_id, **unused_kwargs):
		return None


		# Check that admins can access the editable skill data.
		# Check GET returns 404 when cannot get skill by id.


		def test_skill_publish_handler_fails(self):

		def _publish_skill(*_args):

Fix part of #5134: Increase test coverage for core.controller.skill_editor #5686

Fix part of #5134: Increase test coverage for core.controller.skill_editor #5686

Conversation

ghost commented Sep 23, 2018 • edited by ghost Loading

Checklist

seanlip commented Sep 23, 2018

ghost commented Sep 23, 2018

codecov-io commented Sep 23, 2018 • edited Loading

Codecov Report

seanlip commented Sep 24, 2018

ghost commented Sep 27, 2018

seanlip commented Sep 27, 2018

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

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

ghost Sep 29, 2018 • edited by ghost 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

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

aks681 commented Sep 27, 2018

seanlip left a comment

Choose a reason for hiding this comment

ghost commented Sep 23, 2018 •

edited by ghost

Loading

codecov-io commented Sep 23, 2018 •

edited

Loading

ghost Sep 29, 2018 •

edited by ghost

Loading