Fix part of #5134: Add tests to core_storage_story #5644

JoyLubega · 2018-09-12T13:22:34Z

Add tests to core_storage_story

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.

fix linting on push fix linting on push fix linting on push fix linting befor import order on push fix hanging indent on push fix hanging indent on push

codecov-io · 2018-09-12T14:06:22Z

Codecov Report

Merging #5644 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5644      +/-   ##
===========================================
- Coverage    45.79%   45.74%   -0.05%     
===========================================
  Files          513      515       +2     
  Lines        29872    29954      +82     
  Branches      4519     4522       +3     
===========================================
+ Hits         13679    13702      +23     
- Misses       16193    16252      +59

Impacted Files	Coverage Δ
...ory_editor/StoryEditorNavbarBreadcrumbDirective.js	`6.25% <0%> (-13.75%)`	⬇️
...itor/editor_tab/SkillDescriptionEditorDirective.js	`5.88% <0%> (-0.79%)`	⬇️
...r/editor_tab/SkillMisconceptionsEditorDirective.js	`2.22% <0%> (-0.16%)`	⬇️
...s_and_skills_dashboard/TopicsAndSkillsDashboard.js	`9.37% <0%> (-0.15%)`	⬇️
...es/topic_editor/questions/QuestionsTabDirective.js	`0.71% <0%> (-0.15%)`	⬇️
...d/pages/skill_editor/SkillEditorNavbarDirective.js	`1.81% <0%> (-0.11%)`	⬇️
...itor/editor_tab/SkillConceptCardEditorDirective.js	`1.51% <0%> (-0.1%)`	⬇️
...plates/dev/head/components/CkEditorRteDirective.js	`1.23% <0%> (-0.09%)`	⬇️
...ead/pages/exploration_player/TutorCardDirective.js	`2.27% <0%> (-0.06%)`	⬇️
...pages/state_editor/StateSolutionEditorDirective.js	`1.08% <0%> (-0.03%)`	⬇️
... and 20 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 eb6c0ff...c8829c1. Read the comment docs.

hoangviet1993

Some comments on code styles.

hoangviet1993 · 2018-09-12T17:23:42Z

core/storage/story/gae_models_test.py

+class StoryModelTest(test_utils.GenericTestBase):
+    """Tests for Oppia story models."""
+
+    def test_story_model(self):


Add docstring explaining what is being tested here and in other functions.

hoangviet1993 · 2018-09-12T17:24:50Z

core/storage/story/gae_models_test.py

+        commit_cmds = [{'cmd': 'test_command'}]
+
+        resp = story_models.StoryModel(
+            id='id', title='title',


I think this looks better if each arg is on a new line, like in the test function below.

nithusha21

Hi @JoyLubega. Thanks for the PR. This looks great! A few comments from my side :)

nithusha21 · 2018-09-12T17:24:58Z

core/storage/story/gae_models_test.py

+        commit_message = "test_commit_message"
+        commit_cmds = [{'cmd': 'test_command'}]
+
+        resp = story_models.StoryModel(


I am not sure what resp stands for. Could you use a more descriptive name? Probably story_model, or story_instance or similar would make it more readable.

nithusha21 · 2018-09-12T17:25:17Z

core/storage/story/gae_models_test.py

+
+    def test_story_summary_model(self):
+
+        resp = story_models.StorySummaryModel(


Ditto, regarding variable name

nithusha21 · 2018-09-12T17:26:00Z

core/storage/story/gae_models_test.py

+            language_code='language_code')
+        resp.commit(committer_id, commit_message, commit_cmds)
+        resp = resp.key.get()
+        self.assertEqual(resp.description, "description")


Maybe add checks for the other parameters also?

nithusha21 · 2018-09-12T17:32:14Z

core/storage/story/gae_models_test.py

+            node_count=2,
+            version=1)
+        resp.put()
+        resp = resp.key.get()


This is an interesting way to get an element by it's key. However, I think in most of the codebase, we use something similar to "story_models.StoryModel.get_by_id(story_id)". I would suggest using the get_by_id function as this is how the model gets queried in practice, and we need the test to assert that querying how we normally would should work fine without errors.

Also, one thing to note is that, to get the instance, you are using the already existing instance (which isn't how code would work in practice).

(Please make a similar change in the previous test too)

okay, let me fix that

@nithusha21 the story model does not have "get_by_id". Do I need to first write the function in the models?

Nope. It is a function present in ndb.Model (see here). ndb.Model is inherited by base_model.VersionedModel which is then inherited by the story model. So these functions already exist for every model class.

Thanks alot...

@nithusha21 I have made all the changes but just stuck on this comment you added at the end.
Also, one thing to note is that, to get the instance, you are using the already existing instance (which isn't how code would work in practice).
Help me elaborate on this so that i push my comit.

Hi @JoyLubega, I was referring to the general situation that will occur on the server. The query will be either the id, or any of the parameters of the object (in this case the id). So, the target task is to fetch the object from the datastore given just the id of the object. So I was looking for something along the lines of get_by_id(object_id).

In the earlier code where you did resp.key.get(), it was something like you know that resp is the object, you get its key and then get it again from the datastore. I was just saying that it would be better to do story_models.StoryModel.get_by_id('id').

Okay, thanks it clear now. Let me push the changes

Fix linting issue on push Fix linting issue on push

JoyLubega · 2018-09-20T11:12:56Z

@nithusha21, @hoangviet1993
please review my changes

hoangviet1993

Please reply to each review comments once you submitted your changes.

Thanks!

hoangviet1993 · 2018-09-20T13:28:27Z

core/storage/story/gae_models_test.py

+
+class StoryModelTest(test_utils.GenericTestBase):
+    """Tests for Oppia story models."""
+    def test_story_model(self):


Function docstring?

hoangviet1993 · 2018-09-20T13:28:47Z

core/storage/story/gae_models_test.py

+class StorySummaryModelTest(test_utils.GenericTestBase):
+    """Tests for Oppia story summary models."""
+
+    def test_story_summary_model(self):


Ditto (Re: Function docstring)

hoangviet1993 · 2018-09-20T13:29:39Z

core/storage/story/gae_models_test.py

+            notes='notes',
+            language_code='language_code')
+        story_instance.commit(committer_id, commit_message, commit_cmds)
+        story_by_id = story_models.StoryModel.get_by_id('id')


I feel like there should be a newline here for better readability.

@JoyLubega I don't see the newline over here. Did you push all your changes?

hoangviet1993 · 2018-09-20T13:29:50Z

core/storage/story/gae_models_test.py

+            node_count=2,
+            version=1)
+        story_summary.put()
+        story_summary_by_id = story_models.StorySummaryModel.get_by_id('id')


Insert newline here maybe?

Ditto, regarding newline

nithusha21

Thanks @JoyLubega! This looks great! After making review changes, please reply "Done" on each of the review comments, so that it is easier for a reviewer to track the comments. (As mentioned in point 5 here)

Just one comment from my side.

nithusha21 · 2018-09-21T05:59:54Z

core/storage/story/gae_models_test.py

+
+    def test_story_summary_model(self):
+
+        story_summary = story_models.StorySummaryModel(


Add a "model" at the end of this variable name. We typically refer to domain objects by the noun and the models by noun with a model suffix. If you look at story_domain, you'll see a StorySummary domain class, and typically instances of that class would be referred to as story_summary. So I would prefer you make the clear distinction between models and domain objects by adding a model suffix to this variable.

oppiabot · 2018-10-01T06:05:38Z

Hi @JoyLubega, 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!

…push

nithusha21

Hi @JoyLubega This looks great! Just a few minor details and I think this PR is good to go!

nithusha21 · 2018-10-04T04:16:13Z

core/storage/story/gae_models_test.py

+            notes='notes',
+            language_code='language_code')
+        story_instance.commit(committer_id, commit_message, commit_cmds)
+        story_by_id = story_models.StoryModel.get_by_id('id')


@JoyLubega I don't see the newline over here. Did you push all your changes?

nithusha21 · 2018-10-04T04:17:04Z

core/storage/story/gae_models_test.py

+            node_count=2,
+            version=1)
+        story_summary.put()
+        story_summary_by_id = story_models.StorySummaryModel.get_by_id('id')


Ditto, regarding newline

nithusha21 · 2018-10-04T04:18:55Z

core/storage/story/gae_models_test.py

+    """Tests for Oppia story summary models."""
+
+    def test_story_summary_model(self):
+        """Method to test the story_summary_model."""


I think story_summary_model should be changed to StorySummaryModel (That's how the model class is named right?).

@nithusha21 Done>

nithusha21 · 2018-10-05T13:19:51Z

LGTM. Thanks @JoyLubega! Congrats on your PR!

nithusha21

Sorry about that. Accidentally closed. Approving!

hoangviet1993

Can provide more details in function docstring but that's minor. LGTM!

seanlip · 2018-10-07T05:32:05Z

Merging. Congrats on your first PR, @JoyLubega!

JoyLubega · 2018-10-07T08:18:11Z

Thanks alot @nithusha21 @hoangviet1993 @seanlip

Add story model tests

c65654a

fix linting on push fix linting on push fix linting on push fix linting befor import order on push fix hanging indent on push fix hanging indent on push

JoyLubega requested review from BenHenning and seanlip as code owners September 12, 2018 13:22

JoyLubega mentioned this pull request Sep 12, 2018

Finish writing remaining backend tests #5134

Closed

hoangviet1993 reviewed Sep 12, 2018

View reviewed changes

nithusha21 reviewed Sep 12, 2018

View reviewed changes

Add get_by_id for story model

7e226eb

Fix linting issue on push Fix linting issue on push

hoangviet1993 suggested changes Sep 20, 2018

View reviewed changes

nithusha21 reviewed Sep 21, 2018

View reviewed changes

oppiabot bot added the stale label Oct 1, 2018

kevinlee12 assigned JoyLubega Oct 2, 2018

oppiabot bot removed the stale label Oct 2, 2018

JoyLubega added 7 commits October 2, 2018 12:45

Add finction doc strings to tests in story

a39f6c0

Add finction doc strings to tests in story and fix linting errors on …

24a9188

…push

Fix doc srting error on push

e466c3d

append model to variable name

cba7386

append model to variable name

1eff963

append model to variable name

e4e2e07

Fix linting error in travis

0722bcc

nithusha21 reviewed Oct 4, 2018

View reviewed changes

Add new line

c8829c1

nithusha21 closed this Oct 5, 2018

nithusha21 reopened this Oct 5, 2018

nithusha21 approved these changes Oct 5, 2018

View reviewed changes

hoangviet1993 approved these changes Oct 7, 2018

View reviewed changes

seanlip approved these changes Oct 7, 2018

View reviewed changes

seanlip merged commit f887c01 into oppia:develop Oct 7, 2018

DubeySandeep mentioned this pull request Apr 3, 2019

Finish writing remaining backend tests #6550

Closed


		def test_story_summary_model(self):

		resp = story_models.StorySummaryModel(


		def test_story_summary_model(self):

		story_summary = story_models.StorySummaryModel(

Fix part of #5134: Add tests to core_storage_story #5644

Fix part of #5134: Add tests to core_storage_story #5644

Conversation

JoyLubega commented Sep 12, 2018

Add tests to core_storage_story

Checklist

codecov-io commented Sep 12, 2018 • edited Loading

Codecov Report

hoangviet1993 left a 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

JoyLubega commented Sep 20, 2018

hoangviet1993 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

nithusha21 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 1, 2018

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

nithusha21 commented Oct 5, 2018

nithusha21 left a comment

Choose a reason for hiding this comment

hoangviet1993 left a comment

Choose a reason for hiding this comment

seanlip commented Oct 7, 2018

JoyLubega commented Oct 7, 2018

codecov-io commented Sep 12, 2018 •

edited

Loading

nithusha21 left a comment •

edited

Loading