Fix part of #6550: Write tests for extensions/ and utils #7113

lilithxxx · 2019-07-08T16:51:45Z

Explanation

Fixes part of #6550: Write tests for extensions/ and utils

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-07-08T16:53:55Z

extensions/answer_summarizers/models_test.py

@@ -74,9 +83,6 @@ def _create_state_answers_dict(

    def _get_calculation_instance(self):
        """Requires the existance of the class constant: CALCULATION_ID."""
-        if not hasattr(self, 'CALCULATION_ID'):


These lines are in the test file and are untested.

lilithxxx · 2019-07-08T16:54:12Z

extensions/interactions/base.py

@@ -211,18 +211,6 @@ def html_body(self):
            feconf.INTERACTIONS_DIR, self.id, '%s.html' % self.id))
        return html_templates

-    @property
-    def validator_html(self):


This function is not used anywhere

lilithxxx · 2019-07-08T16:54:44Z

extensions/interactions/base_test.py

@@ -48,11 +48,6 @@
    ('show_generic_submit_button', bool)]


-def mock_get_filename_with_dimensions(filename, unused_exp_id):


This function and its use in this test file is not needed

This means the behavior changed at some point but we didn't update the tests?

Yea that's correct

lilithxxx · 2019-07-08T16:55:03Z

extensions/rich_text_components/components_test.py

@@ -38,14 +38,7 @@ def check_validation(self, rte_component_class, valid_items, invalid_items):
            a TypeError when validated.
        """
        for item in valid_items:
-            try:


These lines are in the test file and are untested.

schema_utils_test.py

lilithxxx · 2019-07-08T16:56:58Z

utils.py

@@ -116,21 +114,20 @@ def get_exploration_components_from_dir(dir_path):
        for filename in files:
            filepath = os.path.join(root, filename)
            if root == dir_path:
-                if filepath.endswith('.DS_Store'):


I changed the structure of the code as I don't know if it will be a good idea to create a .DS_Store file just to test this.

I think this change is fine.

lilithxxx · 2019-07-08T16:57:11Z

utils.py

@@ -144,50 +141,6 @@ def get_exploration_components_from_dir(dir_path):
    return yaml_content, assets_list


-def get_exploration_components_from_zip(zip_file_contents):


This function is not used

nithusha21

Overall looks good... Just some minor comments.

nithusha21 · 2019-07-10T17:00:49Z

core/domain/visualization_registry_test.py

+            re.escape(
+                'For visualization BarChart, expected option names '
+                '[\'x_axis_label\', \'y_axis_label\']; received names []')):
+            self.assertEqual(bar_chart_instance.validate(), 12)


I don't understand why object.validate() returns a value....

That was a mistake. Fixed it!

nithusha21 · 2019-07-10T17:01:45Z

extensions/answer_summarizers/models_test.py

@@ -30,6 +30,15 @@ def test_requires_override_for_calculation(self):
            answer_models.BaseCalculation().calculate_from_state_answers_dict(
                state_answers_dict={})

+    def test_equality_of_hashable_answers(self):
+        hashable_answer_1 = answer_models._HashableAnswer('answer_1')  # pylint: disable=protected-access


Hmmm, I thought the plan was to test protected functions using public interfaces.

I want to test the equality of hashable_answers with the eq method. This is why, an exception needs to be made here

I believe the __eq__ method is what is actually used to implement "a == b" in Python, so you should be able to use the latter below.

nithusha21 · 2019-07-10T17:02:09Z

extensions/answer_summarizers/models_test.py

+        hashable_answer_3 = answer_models._HashableAnswer('answer_1')  # pylint: disable=protected-access
+
+        self.assertFalse(hashable_answer_1.__eq__(hashable_answer_2))
+        self.assertTrue(hashable_answer_1.__eq__(hashable_answer_3))


== doesn't work?

Explained above

nithusha21 · 2019-07-10T17:03:37Z

extensions/interactions/base_test.py

@@ -48,11 +48,6 @@
    ('show_generic_submit_button', bool)]


-def mock_get_filename_with_dimensions(filename, unused_exp_id):


This means the behavior changed at some point but we didn't update the tests?

nithusha21 · 2019-07-10T17:07:58Z

utils.py

@@ -116,21 +114,20 @@ def get_exploration_components_from_dir(dir_path):
        for filename in files:
            filepath = os.path.join(root, filename)
            if root == dir_path:
-                if filepath.endswith('.DS_Store'):


I think this change is fine.

nithusha21 · 2019-07-10T17:08:53Z

utils_test.py

+        utils.require_valid_name(name, 'name_type')
+
+        name = 0
+        with self.assertRaisesRegexp(Exception, 'name_type must be a string.'):


The error message seems wrong? name_type is a string right? name is not a string.

Good catch! The exception message in utils.py was wrong. It was '%s must be a string.' % name_type whereas it should have been '%s must be a string.' % name because we are validating name

lilithxxx · 2019-07-11T13:41:16Z

@nithusha21 PTAL!

nithusha21

LGTM thanks!

vojtechjelinek

LGTM from code owner's perspective. Thanks!

kevinlee12

lgtm!

apb7

LGTM from code owner's perspective, thanks!

oppiabot · 2019-07-14T15:49:53Z

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!

seanlip · 2019-07-15T04:55:35Z

extensions/answer_summarizers/models_test.py

@@ -30,6 +30,15 @@ def test_requires_override_for_calculation(self):
            answer_models.BaseCalculation().calculate_from_state_answers_dict(
                state_answers_dict={})

+    def test_equality_of_hashable_answers(self):
+        hashable_answer_1 = answer_models._HashableAnswer('answer_1')  # pylint: disable=protected-access


I believe the __eq__ method is what is actually used to implement "a == b" in Python, so you should be able to use the latter below.

seanlip · 2019-07-15T04:56:43Z

extensions/interactions/base_test.py

 from core.domain import interaction_registry
 from core.domain import obj_services
 from core.tests import test_utils
 from extensions.interactions import base
+from extensions.interactions.CodeRepl import CodeRepl


Don't do this, use interaction_registry to retrieve the interaction class.

seanlip · 2019-07-15T04:57:14Z

extensions/value_generators/models/generators_test.py

@@ -27,7 +27,7 @@ def test_copier(self):
        generator = generators.Copier()
        self.assertEqual(generator.generate_value({}, **{'value': 'a'}), 'a')
        self.assertEqual(generator.generate_value(
-            {}, **{'value': 'a', 'parse_with_jinja': False}), 'a')
+            None, **{'value': 'a', 'parse_with_jinja': False}), 'a')


Can you explain this change?

I am sorry about this one, shouldn't have modified the existing test. I kept the old one and added a new one.

lilithxxx · 2019-07-16T17:31:41Z

@seanlip @DubeySandeep PTAL!

Rishav Chakraborty added 3 commits July 7, 2019 01:28

Write tests for utils.py

2268914

write tests for extensions/

96aca95

fix lint

b7c305d

lilithxxx requested review from aks681, apb7, brianrodri, DubeySandeep, kevinlee12, nithusha21, seanlip and vojtechjelinek as code owners July 8, 2019 16:51

lilithxxx added the CHANGELOG: Testing coverage label Jul 8, 2019

lilithxxx commented Jul 8, 2019

View reviewed changes

lilithxxx assigned nithusha21 Jul 8, 2019

Rishav Chakraborty added 2 commits July 9, 2019 00:33

Merge branch 'develop' into extensions_test

d8ff86e

fix tests

89cbc67

lilithxxx assigned brianrodri Jul 8, 2019

nithusha21 reviewed Jul 10, 2019

View reviewed changes

Rishav Chakraborty added 2 commits July 11, 2019 18:51

Merge branch 'develop' into extensions_test

f120704

fix changes

cbca8b8

nithusha21 approved these changes Jul 13, 2019

View reviewed changes

nithusha21 removed their assignment Jul 13, 2019

lilithxxx assigned kevinlee12, vojtechjelinek, seanlip, DubeySandeep, aks681 and apb7 Jul 13, 2019

vojtechjelinek approved these changes Jul 13, 2019

View reviewed changes

kevinlee12 approved these changes Jul 13, 2019

View reviewed changes

kevinlee12 unassigned kevinlee12 and vojtechjelinek Jul 13, 2019

apb7 approved these changes Jul 14, 2019

View reviewed changes

apb7 removed their assignment Jul 14, 2019

brianrodri approved these changes Jul 14, 2019

View reviewed changes

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

seanlip reviewed Jul 15, 2019

View reviewed changes

seanlip removed their assignment Jul 15, 2019

aks681 approved these changes Jul 15, 2019

View reviewed changes

aks681 removed their assignment Jul 15, 2019

seanlip assigned lilithxxx and unassigned brianrodri Jul 16, 2019

Rishav Chakraborty added 2 commits July 16, 2019 22:48

merge develop

4633a27

address comments

1c9f698

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

lilithxxx assigned seanlip and unassigned lilithxxx Jul 16, 2019

DubeySandeep assigned lilithxxx and unassigned DubeySandeep Jul 17, 2019

DubeySandeep approved these changes Jul 17, 2019

View reviewed changes

DubeySandeep unassigned lilithxxx Jul 17, 2019

brianrodri merged commit 243e374 into oppia:develop Jul 17, 2019

lilithxxx deleted the extensions_test branch July 17, 2019 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #6550: Write tests for extensions/ and utils #7113

Fix part of #6550: Write tests for extensions/ and utils #7113

lilithxxx commented Jul 8, 2019 •

edited

Loading

lilithxxx Jul 8, 2019

lilithxxx Jul 8, 2019

lilithxxx Jul 8, 2019

nithusha21 Jul 10, 2019

lilithxxx Jul 11, 2019

lilithxxx Jul 8, 2019

lilithxxx Jul 8, 2019

nithusha21 Jul 10, 2019

lilithxxx Jul 8, 2019

nithusha21 left a comment

nithusha21 Jul 10, 2019

lilithxxx Jul 11, 2019

nithusha21 Jul 10, 2019

lilithxxx Jul 11, 2019

seanlip Jul 15, 2019

lilithxxx Jul 16, 2019

nithusha21 Jul 10, 2019

lilithxxx Jul 11, 2019

nithusha21 Jul 10, 2019

nithusha21 Jul 10, 2019

nithusha21 Jul 10, 2019

lilithxxx Jul 11, 2019

lilithxxx commented Jul 11, 2019

nithusha21 left a comment

vojtechjelinek left a comment

kevinlee12 left a comment

apb7 left a comment

oppiabot bot commented Jul 14, 2019

seanlip Jul 15, 2019

seanlip Jul 15, 2019

lilithxxx Jul 16, 2019

seanlip Jul 15, 2019

lilithxxx Jul 16, 2019

lilithxxx commented Jul 16, 2019

		@@ -48,11 +48,6 @@
		('show_generic_submit_button', bool)]


		def mock_get_filename_with_dimensions(filename, unused_exp_id):

		@@ -144,50 +141,6 @@ def get_exploration_components_from_dir(dir_path):
		return yaml_content, assets_list


		def get_exploration_components_from_zip(zip_file_contents):

Fix part of #6550: Write tests for extensions/ and utils #7113

Fix part of #6550: Write tests for extensions/ and utils #7113

Conversation

lilithxxx commented Jul 8, 2019 • edited Loading

Explanation

Checklist

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

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 Jul 11, 2019

nithusha21 left a comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

kevinlee12 left a comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Jul 14, 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

lilithxxx commented Jul 16, 2019

lilithxxx commented Jul 8, 2019 •

edited

Loading