Fix part of #6550: Write backend tests for skill_jobs_one_off, story_jobs_one_off, user_query_jobs_one_off , value_generators_domain, and visualization_registry #6710

lilithxxx · 2019-05-08T08:09:48Z

Fixes part of #6550: Write backend tests for core/domain/skill_jobs_one_off_test.py, core/domain/story_jobs_one_off_test.py, core/domain/user_query_jobs_one_off_test.py , core/domain/value_generators_domain_test.py, and core/domain/visualization_registry_test.py

Explanation

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.
- 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 · 2019-05-08T08:56:20Z

Codecov Report

Merging #6710 into develop will decrease coverage by 1.13%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #6710      +/-   ##
===========================================
- Coverage    95.65%   94.52%   -1.14%     
===========================================
  Files          352      371      +19     
  Lines        48627    49726    +1099     
===========================================
+ Hits         46515    47003     +488     
- Misses        2112     2723     +611

Impacted Files	Coverage Δ
core/tests/gae_suite.py	`74.41% <ø> (ø)`	⬆️
core/domain/value_generators_domain.py	`100% <ø> (+8.1%)`	⬆️
core/domain/user_query_jobs_one_off_test.py	`100% <100%> (ø)`	⬆️
core/domain/skill_jobs_one_off_test.py	`100% <100%> (ø)`	⬆️
core/domain/visualization_registry.py	`100% <100%> (+6.06%)`	⬆️
extensions/interactions/base_test.py	`99.57% <100%> (ø)`	⬆️
core/domain/story_jobs_one_off_test.py	`100% <100%> (ø)`	⬆️
core/domain/visualization_registry_test.py	`100% <100%> (ø)`	⬆️
core/domain/value_generators_domain_test.py	`100% <100%> (ø)`	⬆️
...ensions/value_generators/models/generators_test.py	`100% <100%> (ø)`	⬆️
... and 26 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 82ab4a1...67926dd. Read the comment docs.

brianrodri

Thanks @lilithxxx, I've taken my first pass. PTAL!

core/domain/skill_jobs_one_off_test.py

core/domain/story_jobs_one_off_test.py

core/domain/value_generators_domain.py

nithusha21 · 2019-05-14T17:24:40Z

That's a fairly large PR title, can we trim it by excluding core/domain/ in front of the files, and dropping the .py at the end?

lilithxxx · 2019-05-14T17:45:45Z

@nithusha21 done!

lilithxxx · 2019-05-21T21:46:13Z

@brianrodri @seanlip PTAL!

seanlip · 2019-05-21T23:08:29Z

The backend tests are failing.

lilithxxx · 2019-05-22T16:17:49Z

@seanlip the tests were failing due to:

Incorrect version of selenium used in gae_suite.py (used in adding to sys.path)
An __init__.py file is added to all directories in extensions/ so that the python files present there can be imported. This causes a test to fail which counted the number of files present in the directory(now 5 which was previously 4).

I fixed the tests. PTAL

seanlip · 2019-05-22T19:43:57Z

core/controllers/base_test.py

+                    all_value_generators.append(name)
+
+        for value_generator in all_value_generators:
+            self.assertEqual(


Odd, I thought we were expecting to see two value generators.

Instead of checking the count, can we verify the list of class names instead?

seanlip · 2019-05-22T19:45:36Z

core/domain/skill_jobs_one_off_test.py

+            skill_jobs_one_off.SkillMigrationOneOffJob.enqueue(job_id)
+            self.process_and_flush_pending_tasks()
+
+            output = skill_jobs_one_off.SkillMigrationOneOffJob.get_output(


I think you can pull this and the following statement out of the "with" clause.

seanlip · 2019-05-22T19:46:20Z

core/domain/skill_jobs_one_off_test.py

+                skill_jobs_one_off.SkillMigrationOneOffJob.create_new())
+            skill_jobs_one_off.SkillMigrationOneOffJob.enqueue(job_id)
+            self.process_and_flush_pending_tasks()
+            output = skill_jobs_one_off.SkillMigrationOneOffJob.get_output(


Ditto, minimize what's in "with". Similarly for other files.

seanlip · 2019-05-22T19:51:34Z

core/domain/skill_jobs_one_off_test.py

@@ -169,3 +169,45 @@ def test_migration_job_converts_old_skill(self):
        expected = [[u'skill_migrated',
                     [u'1 skills successfully migrated.']]]
        self.assertEqual(expected, [ast.literal_eval(x) for x in output])
+
+    def test_migration_job_skips_when_new_structure_editor_is_disabled(self):


It took a while to understand why "skips" == "empty output" (which is what this test is actually checking), so at first glance it looked like this test name was misleading. But I think I get it -- we would expect "1 skills successfully migrated" to show up in the success case, and the fact that it doesn't indicates an issue with the job.

It's still worth making this clear, though. Can you add a short comment above the assertEqual below to explain that "If the skill had been successfully migrated, this would include a 'successfully migrated' message. Its absence means that the skill could not be processed."

Ditto for other tests for which this applies.

Done for other tests but not for this once since this test includes new_structure_editor which is now to be removed.

seanlip · 2019-05-22T19:52:00Z

core/domain/skill_jobs_one_off_test.py

+            self.process_and_flush_pending_tasks()
+            output = skill_jobs_one_off.SkillMigrationOneOffJob.get_output(
+                job_id)
+            expected = [[u'validation_error',


Ditto, add explanation of why this implies "skips". Similarly for other files.

core/domain/user_query_jobs_one_off_test.py

core/domain/visualization_registry.py

seanlip · 2019-05-22T20:13:24Z

core/controllers/base_test.py

@@ -712,6 +713,51 @@ def test_controller_class_names(self):
        self.assertGreater(num_handlers_checked, 150)


+class ValueGeneratorNameTests(test_utils.GenericTestBase):


Let's put this in core/domain/value_generators_domain_test.py

extensions/interactions/base_test.py

seanlip · 2019-05-22T20:15:43Z

extensions/value_generators/models/generators_test.py

@@ -32,8 +32,16 @@ def test_copier(self):
            {}, **{'value': '{{a}}', 'parse_with_jinja': False}), '{{a}}')
        self.assertEqual(generator.generate_value(
            {'a': 'b'}, **{'value': '{{a}}', 'parse_with_jinja': True}), 'b')
+        self.assertTrue(generator.get_html_template().startswith(


Here and below, you need to use an expected value that is specific to the value generator under consideration. Otherwise, it's not clear what you are testing -- the snippets you use would appear in pretty much any HTML/JS file.

lilithxxx · 2019-05-23T18:14:25Z

@seanlip addressed your comments. PTAL!

seanlip · 2019-05-23T19:27:33Z

Backend tests are failing.

lilithxxx · 2019-05-23T20:36:00Z

@seanlip the backend tests passed!

core/domain/user_query_jobs_one_off_test.py

core/domain/visualization_registry.py

seanlip · 2019-05-24T00:39:02Z

extensions/value_generators/models/generators_test.py

@@ -32,8 +32,15 @@ def test_copier(self):
            {}, **{'value': '{{a}}', 'parse_with_jinja': False}), '{{a}}')
        self.assertEqual(generator.generate_value(
            {'a': 'b'}, **{'value': '{{a}}', 'parse_with_jinja': True}), 'b')
+        self.assertIn('<object-editor obj-type=', generator.get_html_template())


Here and below, you need to use an expected value that is specific to the value generator under consideration. Otherwise, it's not clear what you are testing -- the snippets you use would appear in pretty much any HTML/JS file.

This comment still applies. Use something that's specific to Copier or to RandomSelector, for all four assertions here. Otherwise the test is a bit meaningless -- for example, every JS file starts with a copyright statement.

lilithxxx · 2019-05-25T23:25:26Z

@seanlip PTAL

seanlip

LGTM!

brianrodri · 2019-05-26T14:00:13Z

Hi @aks681, @apb7, @nithusha21, @kevinlee12, PTAL!

kevinlee12

lgtm!

lilithxxx · 2019-05-28T23:05:13Z

@aks681 @apb7 @nithusha21 you need to take a look as a code owner. Thanks!

nithusha21

LGTM as a codeowner

lilithxxx · 2019-05-30T21:21:58Z

@aks681 @apb7 PTAL

apb7

LGTM from code owner's perspective. Thanks!

aks681

lgtm

Added tests

eec0b37

lilithxxx requested review from aks681, brianrodri and seanlip as code owners May 8, 2019 08:09

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

DubeySandeep assigned brianrodri May 8, 2019

brianrodri suggested changes May 9, 2019

View reviewed changes

core/domain/skill_jobs_one_off_test.py Outdated Show resolved Hide resolved

core/domain/skill_jobs_one_off_test.py Outdated Show resolved Hide resolved

core/domain/story_jobs_one_off_test.py Show resolved Hide resolved

core/domain/value_generators_domain.py Show resolved Hide resolved

Rishav Chakraborty added 2 commits May 9, 2019 06:06

Addressed comments

cdeecd3

Merge branch 'develop' into backends_test_4

a2d0111

Rishav Chakraborty added 2 commits May 18, 2019 17:51

Merge branch 'develop' into backends_test_4

4b62e9a

Merge branch 'develop' into backends_test_4

172de67

lilithxxx requested review from apb7, kevinlee12 and nithusha21 as code owners May 21, 2019 21:45

Add value generators test

6f00411

Fix backend tests

e50db2a

seanlip requested changes May 22, 2019

View reviewed changes

Rishav Chakraborty added 2 commits May 23, 2019 22:49

Merge branch 'develop' into backends_test_4

591a7c2

Addressed comments

67fc3df

Merge branch 'develop' into backends_test_4

4f6debf

Fix backend test

b5f9eb9

seanlip reviewed May 24, 2019

View reviewed changes

Merge branch 'develop' into backends_test_4

17dcaa2

Addressed comments

67926dd

seanlip approved these changes May 26, 2019

View reviewed changes

brianrodri approved these changes May 26, 2019

View reviewed changes

brianrodri assigned kevinlee12, nithusha21, aks681 and apb7 and unassigned brianrodri May 26, 2019

kevinlee12 approved these changes May 26, 2019

View reviewed changes

kevinlee12 removed their assignment May 26, 2019

nithusha21 reviewed May 29, 2019

View reviewed changes

nithusha21 approved these changes May 29, 2019

View reviewed changes

nithusha21 removed their assignment May 29, 2019

apb7 approved these changes Jun 1, 2019

View reviewed changes

apb7 removed their assignment Jun 1, 2019

aks681 approved these changes Jun 1, 2019

View reviewed changes

aks681 merged commit 2046eb0 into oppia:develop Jun 1, 2019

lilithxxx deleted the backends_test_4 branch June 1, 2019 15:13

		@@ -712,6 +713,51 @@ def test_controller_class_names(self):
		self.assertGreater(num_handlers_checked, 150)


		class ValueGeneratorNameTests(test_utils.GenericTestBase):

Fix part of #6550: Write backend tests for skill_jobs_one_off, story_jobs_one_off, user_query_jobs_one_off , value_generators_domain, and visualization_registry #6710

Fix part of #6550: Write backend tests for skill_jobs_one_off, story_jobs_one_off, user_query_jobs_one_off , value_generators_domain, and visualization_registry #6710

Conversation

lilithxxx commented May 8, 2019 • edited Loading

Explanation

Checklist

codecov-io commented May 8, 2019 • edited by codecov bot Loading

Codecov Report

brianrodri left a comment

Choose a reason for hiding this comment

nithusha21 commented May 14, 2019

lilithxxx commented May 14, 2019

lilithxxx commented May 21, 2019

seanlip commented May 21, 2019

lilithxxx commented May 22, 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

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 May 23, 2019

seanlip commented May 23, 2019

lilithxxx commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilithxxx commented May 25, 2019

seanlip left a comment

Choose a reason for hiding this comment

brianrodri commented May 26, 2019 • edited Loading

kevinlee12 left a comment

Choose a reason for hiding this comment

lilithxxx commented May 28, 2019

nithusha21 left a comment

Choose a reason for hiding this comment

lilithxxx commented May 30, 2019

apb7 left a comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

lilithxxx commented May 8, 2019 •

edited

Loading

codecov-io commented May 8, 2019 •

edited by codecov bot

Loading

lilithxxx commented May 22, 2019 •

edited

Loading

brianrodri commented May 26, 2019 •

edited

Loading