fix part of #3954 - LearnerDashboardPage.js #4138

aks681 · 2017-12-01T21:35:35Z

I made a file LearnerDashboardPage.js that incorporates all the learner page objects.
PTAL @brianrodri

codecov-io · 2017-12-01T22:53:33Z

Codecov Report

Merging #4138 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4138      +/-   ##
===========================================
- Coverage    43.88%   43.88%   -0.01%     
===========================================
  Files          359      360       +1     
  Lines        22401    22398       -3     
  Branches      3559     3561       +2     
===========================================
- Hits          9831     9829       -2     
+ Misses       12570    12569       -1

Impacted Files	Coverage Δ
extensions/visualizations/visualizations.js	`10.71% <0%> (-0.83%)`	⬇️
...exploration_editor/statistics_tab/StatisticsTab.js	`1.8% <0%> (-0.11%)`	⬇️
...n_editor/editor_tab/SolutionVerificationService.js	`100% <0%> (ø)`	⬆️
...tion_editor/StateEditorTutorialFirstTimeService.js	`5.26% <0%> (ø)`
...ges/exploration_editor/editor_tab/StateSolution.js	`1.56% <0%> (+0.06%)`	⬆️
...tes/dev/head/components/SolutionEditorDirective.js	`2.7% <0%> (+0.2%)`	⬆️
.../exploration_editor/editor_tab/ResponsesService.js	`41.04% <0%> (+0.69%)`	⬆️
...ev/head/pages/exploration_editor/EditorServices.js	`39.91% <0%> (+0.88%)`	⬆️

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 7fa2bcb...4c3195c. Read the comment docs.

seanlip

Thanks for the changes. I took a review pass -- PTAL.

seanlip · 2017-12-03T18:46:27Z

core/tests/protractor/learnerDashboard.js

@@ -56,16 +60,9 @@ describe('Learner dashboard functionality', function() {
    browser.waitForAngular();
    element(by.css('.protractor-test-create-activity')).click();
    // Create new collection.
-    element(by.css('.protractor-test-create-collection')).click();
+    learnerDashboardPage.createExploration();


This action is not something that's being done on the learner dashboard page, so it should not be part of the learner dashboard page object.

ok, I have removed it

seanlip · 2017-12-03T18:46:32Z

core/tests/protractor/learnerDashboard.js

-    collectionEditor.setObjective('This is a test collection.');
-    collectionEditor.setCategory('Algebra');
-    collectionEditor.saveChanges();
+    learnerDashboardPage.publishExploration();


Ditto. This looks more like a job for the CollectionEditorPage.

Done, I have changed it

seanlip · 2017-12-03T18:48:11Z

core/tests/protractor/learnerDashboard.js

    browser.waitForAngular();
-    element(by.css('.protractor-test-completed-section')).click();
+    learnerDashboardPage.completedProfileTest();


If this represents a click, the name of the method should indicate that. E.g. learnerDashboardPage.moveToCompletedSection() or learnerDashboardPage.openCompletedSection() (or whatever the click is actually doing semantically; I can't tell from the name that's being given to it here).

Done, I have renamed all occurrences of such names.

seanlip · 2017-12-03T18:59:06Z

core/tests/protractor/learnerDashboard.js

    general.acceptAlert();
    browser.waitForAngular();
    general.waitForSystem();
    browser.ignoreSynchronization = false;
-    element(by.css('.protractor-test-incomplete-collection-section')).click();
+    learnerDashboardPage.incompleteCollectionSection();


Ditto: method names should start with verbs and accurately describe what they do semantically.

Done, I have renamed it.

seanlip · 2017-12-03T18:59:19Z

core/tests/protractor/learnerDashboard.js

-      '.protractor-test-collection-summary-tile-title')).first(
-    ).getText()).toMatch('Test Collection');
+    expect(
+      learnerDashboardPage.summaryTile()


Done, I have renamed it.

seanlip · 2017-12-03T18:59:55Z

core/tests/protractor/learnerDashboard.js

@@ -210,14 +207,14 @@ describe('Learner dashboard functionality', function() {
    // Subscribe to both the creators.
    browser.get('/profile/creator1learnerDashboard');
    browser.waitForAngular();
-    element(by.css('.protractor-test-subscription-button')).click();
+    learnerDashboardPage.clickSubscriptionButton();


This name is good -- but this isn't the learnerDashboardPage, it's the profilePage, right?

ok, thanks for the suggestions, will make the necessary changes.

aks681 · 2017-12-03T20:27:57Z

@seanlip Did the changes and also added some functions which I missed before.

seanlip · 2017-12-03T20:28:35Z

Thanks @aks681. One request: please could you reply to all comments, following the instructions in step 5 here? In particular, for comments that you've addressed, please reply "Done." -- this helps keep track of which comments have been addressed and which haven't. Thanks!

aks681 · 2017-12-03T20:40:06Z

ok, will do, sorry about that.

seanlip · 2017-12-03T21:06:01Z

core/tests/protractor/learnerDashboard.js

    general.acceptAlert();
    browser.waitForAngular();
    general.waitForSystem();
    browser.ignoreSynchronization = false;
-    element(by.css('.protractor-test-incomplete-collection-section')).click();
+    learnerDashboardPage.navigateToIncompleteCollection();


navigateToIncompleteCollectionSection (you're navigating to a section, not to a collection)

Ya, I thought about this, but thought name might become too long. Will change that.

seanlip · 2017-12-03T21:07:08Z

core/tests/protractor/learnerDashboard.js

-      '.protractor-test-collection-summary-tile-title')).first(
-    ).getText()).toMatch('Test Collection');
+    expect(
+      learnerDashboardPage.openSummaryTile()


open...() doesn't have a connotation of "return a value". Maybe it should be "getTitleOfSummaryTile()" or similar, though that's just an example -- you'd need to check what exactly is being retrieved here and include that in the method name.

Yes, that is what is being retrieved here, will change that.

seanlip · 2017-12-03T21:07:43Z

core/tests/protractor/learnerDashboard.js

    browser.waitForAngular();
    general.waitForSystem();
-    element(by.css('.protractor-test-subscriptions-section')).click();
+    learnerDashboardPage.navigateToSubscriptionsSection();
    browser.waitForAngular();
    expect(element.all(by.css(


I guess this stuff should also be factored into the learner dashboard page?

As it was just after a learnerDashboard.get(), I assumed it to be a part of learner dashboard itself.

That's the point I was making, though. If it's part of the learner dashboard page, shouldn't it be refactored into the LearnerDashboardPage object?

Yes, thats what I did. Is there something more?

What I'm saying is that this line (expect(element.all(by.css(...))).toMatch(...);) should be wrapped in a method on the LearnerDashboardPage object, so that this line changes to something like learnerDashboardPage.someMethodName('creator...'). It doesn't look like that's been done.

Oh, ok, will add that

seanlip · 2017-12-03T21:08:06Z

core/tests/protractor_utils/LearnerDashboardPage.js

+    element(by.css('.protractor-test-completed-section'));
+  var feedbackSection =
+    element(by.css('.protractor-test-feedback-section'));
+  var completedCollection =


These are sections; their names should end with Section. Ditto below.

ok, will change those.

seanlip · 2017-12-03T21:08:31Z

core/tests/protractor_utils/LearnerDashboardPage.js

+    element(by.css('.protractor-test-incomplete-collection-section'));
+  var subscriptionsSection =
+    element(by.css('.protractor-test-subscriptions-section'));
+  var summaryTile =


Is this the tile, or its title? The CSS name doesn't seem to match the variable name.

It's important to be precise when naming, otherwise developers will get confused.

ok, misread the name, will change that

seanlip · 2017-12-03T21:08:40Z

core/tests/protractor_utils/LearnerDashboardPage.js

+
+  this.navigateToCompletedSection = function() {
+    completedSection.click();
+  }


Missing semicolons, here and below.

Oh, didn't see those. Will add them.

seanlip · 2017-12-03T21:56:44Z

core/tests/protractor/learnerDashboard.js

    browser.waitForAngular();
-    element(by.css('.protractor-test-completed-section')).click();
+    learnerDashboardPage.navigateToCompletedSection();


Just to check -- is this completed collections, completed explorations, or something else?

I think its a section of completed explorations and collections. In LearnerDashboard.js, of /learner_dashboard it is called above the subsection that calls both of them.

seanlip · 2017-12-03T21:56:52Z

core/tests/protractor/learnerDashboard.js

    general.acceptAlert();
    browser.waitForAngular();
    general.waitForSystem();
    browser.ignoreSynchronization = false;
-    element(by.css('.protractor-test-incomplete-collection-section')).click();
+    learnerDashboardPage.navigateToIncompleteCollectionSection();


Let's make this ...IncompleteCollectionsSection (note the plural), since the section contains incomplete collections

seanlip · 2017-12-03T21:57:02Z

core/tests/protractor/learnerDashboard.js

    browser.waitForAngular();
    general.waitForSystem();
-    element(by.css('.protractor-test-completed-collection-section')).click();
+    learnerDashboardPage.navigateToCompletedCollectionSection();


seanlip · 2017-12-03T21:58:00Z

core/tests/protractor/learnerDashboard.js

    browser.waitForAngular();
-    element(by.css('.protractor-test-feedback-section')).click();
+    learnerDashboardPage.navigateToFeedbackSection();
    browser.waitForAngular();
    expect(element.all(by.css(


This also seems like something that should be part of the LearnerDashboardPage object?

Basically, anything that involves fiddling around with elements should be part of the page object, so that the protractor test itself doesn't have to mess around with the details.

Yes, I added that in the learnerDashboardPage object already. Is there something else?

I don't think it's done, or rather, if it is, then this line should be learnerDashboardPage.something(...). See previous comment.

Yes, saw it, will add that.

seanlip

This is looking great, @aks681. Just a couple more things and I think we're done!

seanlip · 2017-12-04T08:43:02Z

core/tests/protractor_utils/LearnerDashboardPage.js

+ * tests.
+ */
+
+var collectionEditor = require('./collectionEditor.js');


Why is this here?

It was there as I was calling it before. Forgot to remove it. Will change that.

seanlip · 2017-12-04T08:43:50Z

core/tests/protractor/learnerDashboard.js

    general.acceptAlert();
    browser.waitForAngular();
    general.waitForSystem();
    browser.ignoreSynchronization = false;
-    element(by.css('.protractor-test-incomplete-collection-section')).click();
+    learnerDashboardPage.navigateToIncompleteCollectionsSection();
    browser.waitForAngular();


It might be better to put these browser.waitForAngular() calls at the end of their respective functions in the page object, if they're always going to be called. This will also help simplify the main protractor test.

Ditto for general.waitForSystem(), where relevant.

If there are are other calls to learnerDashboardPage that are repeating for all such instances, shall I put it inside page object also?

Sorry, I don't understand. Could you elaborate / give an example?

after both instances of navigateToIncompleteCollectionsSection(), expectTitleOfSummaryTileToMatch() is called, so can I put this call as this.expectTitleOfSummaryTileToMatch() inside the navigateToIncompleteCollectionsSection() function?

Ah, I see. No, don't do that.

The way to think about this is that these are the actions a user would do. A user wouldn't really be doing stuff at the level of "wait for angular". But they may take the actions "navigate" and "expect title to match" independently, so these should be independent actions in the page object.

Ok, thanks.

seanlip

LGTM. Thanks @aks681!

aks681 · 2017-12-05T07:09:54Z

Thank You!

aks681 added 4 commits December 2, 2017 00:03

fix part of oppia#3954 - Learner Dashboard Page

3eb5ecd

fixed lint errors

191a1ed

removed LEARNER_DASHBOARD_URL from general.js

268a80e

fixes to learnerDashboard.js

111b314

DubeySandeep requested review from shubha1593 and DubeySandeep December 2, 2017 02:41

DubeySandeep assigned shubha1593 and DubeySandeep Dec 2, 2017

DubeySandeep requested review from seanlip and removed request for shubha1593 and DubeySandeep December 3, 2017 10:43

DubeySandeep unassigned shubha1593 and DubeySandeep Dec 3, 2017

seanlip requested changes Dec 3, 2017

View reviewed changes

some fixes and more functions added

19bc125

seanlip requested changes Dec 3, 2017

View reviewed changes

made the review changes

4f4f6e9

seanlip requested changes Dec 3, 2017

View reviewed changes

aks681 added 2 commits December 4, 2017 12:37

changed page object names

9603377

added subscription and feedback sections

00ced47

seanlip requested changes Dec 4, 2017

View reviewed changes

made the review changes

4c3195c

seanlip approved these changes Dec 4, 2017

View reviewed changes

seanlip merged commit 7928a4d into oppia:develop Dec 4, 2017

brianrodri mentioned this pull request Dec 4, 2017

Refactor functions in core/tests/protractor_util into Page Objects #3954

Closed

10 tasks

aks681 deleted the refactor_into_page_objects branch December 5, 2017 07:10

fix part of #3954 - LearnerDashboardPage.js #4138

fix part of #3954 - LearnerDashboardPage.js #4138

Conversation

aks681 commented Dec 1, 2017

codecov-io commented Dec 1, 2017 • edited Loading

Codecov Report

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

aks681 Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 Dec 3, 2017 • 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

aks681 commented Dec 3, 2017

seanlip commented Dec 3, 2017

aks681 commented Dec 3, 2017

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 Dec 3, 2017 • 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

aks681 Dec 3, 2017 • 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

aks681 Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 Dec 4, 2017 • 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

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

seanlip left a comment

Choose a reason for hiding this comment

aks681 commented Dec 5, 2017

codecov-io commented Dec 1, 2017 •

edited

Loading

aks681 Dec 3, 2017 •

edited

Loading

aks681 Dec 3, 2017 •

edited

Loading

aks681 Dec 3, 2017 •

edited

Loading

aks681 Dec 3, 2017 •

edited

Loading

aks681 Dec 4, 2017 •

edited

Loading

aks681 Dec 4, 2017 •

edited

Loading