Improving the dev workflow: Setup browserstack, add necessary configuration files and tests for mobile #5340

apb7 · 2018-07-20T19:53:44Z

Explanation

Fixes #2991 and Fixes #5363

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.

…stack-integration

seanlip · 2018-07-20T19:57:26Z

core/tests/.browserstack.env.example

@@ -0,0 +1,2 @@
+USERNAME={Browserstack username}


what's this? is it needed? probably needs some explanation on the process for using the actual credentials...

This is the sample environment file. An similar file named .browserstack.env with the actual credentials is required to run the tests locally on Browserstack. I will add some explanation for this. Thanks!

Oh ok. But the tests are going to run on Travis too, right?

Yes, they will continue to run as they now. I have also shared a doc with you regarding the plan.

Hi @apb7 , I have been looking at this PR for a while now. Would it be possible for me to take a look at this doc as well?

codecov-io · 2018-07-21T00:49:23Z

Codecov Report

Merging #5340 into develop will decrease coverage by <.01%.
The diff coverage is 32%.

@@             Coverage Diff             @@
##           develop    #5340      +/-   ##
===========================================
- Coverage     47.3%   47.29%   -0.01%     
===========================================
  Files          496      496              
  Lines        28436    28442       +6     
  Branches      4343     4344       +1     
===========================================
  Hits         13452    13452              
- Misses       14984    14990       +6

Impacted Files	Coverage Δ
...tes/dev/head/components/SolutionEditorDirective.js	`12.5% <0%> (ø)`	⬆️
...ts/summary_tile/ExplorationSummaryTileDirective.js	`1.53% <0%> (ø)`	⬆️
...oration_editor/statistics_tab/PieChartDirective.js	`6.25% <0%> (ø)`	⬆️
...tion_editor/ExplorationObjectiveEditorDirective.js	`20% <0%> (ø)`	⬆️
...v/head/pages/learner_dashboard/LearnerDashboard.js	`2.27% <0%> (ø)`	⬆️
...dev/head/services/SpeechSynthesisChunkerService.js	`23.28% <0%> (ø)`	⬆️
core/templates/dev/head/pages/library/Library.js	`24.34% <0%> (ø)`	⬆️
...nts/summary_tile/CollectionSummaryTileDirective.js	`14.28% <0%> (ø)`	⬆️
core/templates/dev/head/services/SearchService.js	`38.05% <0%> (-2.14%)`	⬇️
...itor/editor_tab/SkillConceptCardEditorDirective.js	`1.61% <0%> (ø)`	⬆️
... and 5 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 a9080d4...4e14f53. Read the comment docs.

kevinlee12

Hi @apb7, left some comments, ptal!

kevinlee12 · 2018-07-21T03:05:21Z

core/tests/protractor-browserstack.conf.js

+  // Code to start browserstack local before start of test
+  beforeLaunch: function(){
+    // eslint-disable-next-line no-console
+    console.log('Connecting local');


Connecting browserstack local

kevinlee12 · 2018-07-21T03:06:01Z

core/tests/protractor-browserstack.conf.js

+  // This can be changed via the command line as:
+  //   --params.login.user 'Joe'
+  params: {
+    login: {


Do we need this?

kevinlee12 · 2018-07-21T03:06:50Z

core/tests/protractor-browserstack.conf.js

+  // ----- Options to be passed to mocha -----
+  //
+  // See the full list at http://visionmedia.github.io/mocha/
+  mochaOpts: {


Can we remove this?

kevinlee12 · 2018-07-21T03:06:55Z

core/tests/protractor-browserstack.conf.js

+  },
+
+  // ----- Options to be passed to cucumber -----
+  cucumberOpts: {


…, in LibraryPage object

seanlip

Seems better on first glance but grepping for browser.isMobile uncovered a couple more things. Once you fix them I'll do a full review.

Thanks!

seanlip · 2018-08-03T01:14:56Z

core/tests/protractor/libraryFlow.js

+
+  var rateExploration = function() {
+    if (browser.isMobile) {
+      var adminPage = new AdminPage.AdminPage();


This stuff doesn't look like a single line

Here we use a different function to login for the mobile: users.createAndLoginAdminUserMobile. Therefore, the difference in lines. We can however use the same function for desktop too. How should we go about this then? Shall we rename it to something like: createAndLoginAdminUserViaAdminCheckboxOnly or something on that lines?

I guess I don't understand why the user is admin in one case and non-admin in the other case, or why the usernames and email addresses used are different. Can we make them the same? Ideally you would not need any branching at all except for reloadExploration() vs createAndPublishExploration().

Okay, so we can have a single admin user and then reload or create and publish the exploration but we should use users.createAndLoginAdminUserMobile function instead of users.createAndLoginAdminUser to prevent any popups of the admin page and rename the function appropriately. Does that sounds good? Thanks!

So, I didn't actually understand the comment in the code. What popup exactly is appearing?

OK. For clarity, think of the admin page as the "superadmin page".

I get that, on mobile, stuff is loaded from the superadmin page. But I think you solved that in all the other tests already quite cleanly, so I'm not sure why this test is special in that regard...?

Okay, understood. This was the approach I had adopted:

users.createUser('random@gmail.com', 'random'); users.login('random@gmail.com', true); if (browser.isMobile) { var adminPage = new AdminPage.AdminPage(); adminPage.reloadExploration('protractor_mobile_test_exploration.yaml'); } else { workflow.createAndPublishExploration( EXPLORATION_TITLE, EXPLORATION_CATEGORY, EXPLORATION_OBJECTIVE, EXPLORATION_LANGUAGE ); } ... ... users.logout();

This seems reasonable :) Try to get the stuff within each branch to a single call, though.

Sounds good. I will move the admin page initialisation to outside the branching so as to reduce this to a single call. Thanks :)

Done, thanks!

seanlip · 2018-08-03T01:16:47Z

core/tests/protractor_utils/LibraryPage.js

-    searchInput.sendKeys(explorationTitle);
+    // The library page has two search bar input elements.
+
+    if (browser.isMobile) {


Should not duplicate so much code with findCollection(). Have both methods delegate to an internal method for selectLibraryTile(tileName) perhaps.

Actually the name is ambiguous. Looks like it's just entering input in search bar, does it hit enter? Maybe submitSearchQuery() would be better.

Created a separate function, _submitSearchQuery(searchQuery) for this and moved the duplicate code under it and then called it appropriately. Done, thanks!

oppiabot · 2018-08-03T07:42:19Z

Hi @apb7. 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!

apb7 · 2018-08-03T09:20:24Z

@seanlip: I have addressed both the review comments. Can you please take another look? Thanks!

seanlip

Almost there!

seanlip · 2018-08-03T09:13:14Z

core/templates/dev/head/services/RteHelperServiceSpec.js

@@ -0,0 +1,58 @@
+// Copyright 2018 The Oppia Authors. All Rights Reserved.


Hm. Did this get removed in develop? Surprised to see it here -- might want to check. /cc @bansalnitish

Yup, it's removed.

Whoops! Removed this. Thanks!

seanlip · 2018-08-03T09:24:38Z

core/tests/protractor/learnerFlow.js

+    libraryPage = new LibraryPage.LibraryPage();
+    learnerDashboardPage = new LearnerDashboardPage.LearnerDashboardPage();
+
+    if (browser.isMobile) {


In all such if/else statements, both branches should have the same functionality. This isn't the case here.

It might be better to do the branching on a per-test basis.

Done, followed the per-test basis. Thanks!

seanlip · 2018-08-03T09:26:01Z

core/tests/protractor/learnerFlow.js

+
+  it('visits the exploration player and plays the correct exploration',
+    function() {
+      if (!browser.isMobile) {


Should have corresponding branch for mobile that is clearly equivalent to desktop branch.

Ditto in all other tests in this file. Ditto in other files if applicable.

Done, the per-test basis resolved this. Thanks!

seanlip · 2018-08-03T09:29:28Z

core/tests/protractor/learnerFlow.js

+  afterEach(function() {
+    if (browser.isMobile) {
+      general.checkForConsoleErrors([
+        // TODO(apb7): Remove these when https://github.com/oppia/oppia/issues/5363 is resolved.


OK -- we really need to find a way around this. The console error log is getting a bit ridiculous...

These error logs are generated by us. (Search for "SearchQuery" in the codebase.) What are the exact values of $('.oppia-search-bar-input').val().trim() and searchQuery, respectively? We should be able to get more insight than what's currently in #5363.

Okay, so the problem here was, we have two oppia-search-bar-input tags -- the first one is visible for desktop and the second one is visible for mobile devices. $('.oppia-search-bar-input').val().trim() takes into consideration only the first search bar input tag which, in case of mobile, has no value since the mobile devices use the second search bar input tag and therefore, the searchQuery does not match and we get the console errors. I think this also conveys that this error must have been introduced in the PR stage only, that is, in PR #4807, contrary to our belief that it was introduced by some later update.
I have fixed this error by checking all search bar input tags and throwing the console error if the value of none of them match the searchQuery. This fixes the issue. Thanks!

seanlip · 2018-08-03T09:32:02Z

core/tests/protractor/loginFlow.js

+var users = require('../protractor_utils/users.js');
+var waitFor = require('../protractor_utils/waitFor.js');
+
+describe('Login flow', function() {


Looks more like a test for the profile dropdown menu to me...

We have another nested describe block for profile dropdown menu here.
Do we still need to change this description?

Could we rename it (and the file) to something like "navigation" perhaps? This file is, like, 90% non-login stuff, so the name seems misleading.

Yes, I agree. Since we already have a file named navigation for mobile, shall I change this to profileFlow since it's mostly concerned about navigation using the profile menu?

profileMenuFlow perhaps? But otherwise sgtm.

Done, renamed to profileMenuFlow. Thanks!

seanlip · 2018-08-03T09:34:00Z

core/tests/protractor_utils/AdminPage.js

+    var reloadCollectionButtons = element.all(by.css(
+      '.protractor-test-reload-collection-button'));
+
+    var explorationTitleElement = function(explorationElement) {


Start functions with get. Ditto for explorationElementReloadButton.

Done, thanks!

seanlip · 2018-08-03T09:35:29Z

core/tests/protractor_utils/LibraryPage.js

+      // page is rendered for mobile device.
+
+      // get function is a zero-based index.
+      var mobileSearchInput = searchInputs.get(1);


Reduce branching:

// ... var searchInput = (browser.isMobile ? searchInputs.get(1) : searchInputs.first()); searchInput.clear(); searchInput.sendKeys(searchQuery);

Done, thanks!

…stack-integration

apb7 · 2018-08-03T19:57:08Z

@seanlip: I have addressed all review comments. Can you please take another look? Thanks!

seanlip · 2018-08-03T21:59:30Z

core/tests/protractor/learnerFlow.js

@@ -403,9 +399,13 @@ describe('Learner dashboard functionality', function() {
    // 'Test Collection'.


Why is this test only for desktop? Mobile and desktop should test the same functionality.

Okay, let me once state what this part of the test does. We go to the creator dashboard and then navigate to the collection editor page by clicking on the "Collections" tab. We then chose a collection and then we chose an exploration to add to the existing collection.
It is only for desktop because of the following reasons:

A user can only add an existing exploration to a collection he has created. For desktop, the user creates a collection. Later on, it adds to the same collection it had created earlier. But for mobile, we load a collection and not create it. Therefore, the user cannot add an existing exploration to the loaded collection since the collection is predefined.

This involves the collection editor page, which I think has certain components which are not mobile friendly.

Creating and later on, editing a collection involves an admin user and not a super admin. We sign-in as super admin for mobile.

Also, the collection editor page where we add an existing exploration says that its in beta. So I am not too sure how it will be affected in case of mobile devices.

Therefore, this part was kept under desktop.

OK, thanks for explaining. Should we keep this part in protractor_desktop then (and add comments explaining why)?

(I think in general stuff in protractor/ is stuff that gets tested on both mobile and desktop.)

Actually this is just a part of the same test -- it checks the same functionality, that is, after adding an existing exploration to the collection which was earlier completed, the collection now appears under the incomplete collection section. Therefore, I think we can keep it here and add the explanation as to why it is a desktop only thing. Also, if we place this in a separate file, it would again require the same setup -- creating two explorations and a collection and then testing this. Shall we add the explanation here only?

OK, yep that's fine.

seanlip

All good, just one more thing!

apb7 · 2018-08-03T23:48:42Z

@seanlip: Please take another look :) Thanks!

seanlip

LGTM. Thanks!

apb7 · 2018-08-04T00:08:15Z

Thanks, Sean :)

apb7 · 2018-08-04T18:25:42Z

Merging this, thanks!

apb7 added 12 commits July 19, 2018 19:30

Basic Setup

59adedb

Add protractor-production to config-file-paths

a785896

Add copyright notice

60a731c

Modify production conf

dd93d40

Try out linux platform

0bff650

Change platform to mac

1c09375

Merge branch 'develop' of https://github.com/oppia/oppia into browser…

404e8d4

…stack-integration

Revert Travis changes

f07b6bb

Add sample env file for browserstack

8b4bc4b

Add configuration file for browserstack

9c7d3b4

Other minor changes

14f5577

Add env example to config files

16e0cd6

apb7 requested a review from kevinlee12 July 20, 2018 19:53

seanlip reviewed Jul 20, 2018

View reviewed changes

Add explanation for the process

1ac8185

kevinlee12 reviewed Jul 21, 2018

View reviewed changes

kevinlee12 assigned apb7 Jul 21, 2018

apb7 changed the title ~~Improving the dev workflow: Setup browserstack and necessary configuration files~~ [WIP] Improving the dev workflow: Setup browserstack and necessary configuration files Jul 21, 2018

apb7 added 11 commits July 23, 2018 18:12

Add isMobile variable to existing conf file

bfb72ea

Increase timeout for mobile device

0a6f6f5

Remove unused until variable

ffb648d

Add more options to example environment file

008326b

Refactor learnerDashboard

a3c48a9

Add subscriptions end-to-end tests

40eeb8b

Add export for completeSignup function

a6015d1

Add reload functions in AdminPage object

4bf14cc

Add helper functions for mobile in CreatorDashboardPage object

d406e0d

Add demo test exploration for mobile and exploration id in feconf.py

3835b02

Make findExploration and findCollection functions suitable for mobile…

88f0632

…, in LibraryPage object

seanlip reviewed Aug 3, 2018

View reviewed changes

oppiabot bot added PR: don't merge - HAS MERGE CONFLICTS and removed PR: don't merge - HAS MERGE CONFLICTS labels Aug 3, 2018

apb7 added 2 commits August 3, 2018 13:49

Create submitSearchQuery function

37fc3a3

Reduce branching in libraryFlow

6a53974

seanlip requested changes Aug 3, 2018

View reviewed changes

apb7 added 2 commits August 3, 2018 18:36

Merge branch 'develop' of https://github.com/oppia/oppia into browser…

b253c5a

…stack-integration

Remove file

89f31bb

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Aug 3, 2018

apb7 added 6 commits August 4, 2018 00:25

Unification and fixes

b42cffd

Fix search service

99e4474

Add necessary tags

ba52b39

Reduce branching and other minor tweaks

8096443

Rename loginFlow to profileFlow and necessary changes

796ff4c

Rename profileFlow to profileMenuFlow and necessary changes

66f71b2

seanlip reviewed Aug 3, 2018

View reviewed changes

Add explanation for desktop part of test

ad88702

seanlip added the PR: LGTM label Aug 3, 2018

seanlip approved these changes Aug 3, 2018

View reviewed changes

Remove console error msgs

4e14f53

apb7 merged commit 0b5692a into develop Aug 4, 2018

apb7 deleted the browserstack-integration branch August 4, 2018 18:26

brianrodri mentioned this pull request Mar 16, 2021

Fix #11462: Migrate to Firebase authentication #12172

Merged

4 tasks

		@@ -0,0 +1,58 @@
		// Copyright 2018 The Oppia Authors. All Rights Reserved.

		@@ -403,9 +399,13 @@ describe('Learner dashboard functionality', function() {
		// 'Test Collection'.

Improving the dev workflow: Setup browserstack, add necessary configuration files and tests for mobile #5340

Improving the dev workflow: Setup browserstack, add necessary configuration files and tests for mobile #5340

Conversation

apb7 commented Jul 20, 2018 • 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

codecov-io commented Jul 21, 2018 • edited Loading

Codecov Report

kevinlee12 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Aug 3, 2018

apb7 commented Aug 3, 2018

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

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

apb7 commented Aug 3, 2018

seanlip Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

apb7 Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 Aug 3, 2018 • edited Loading

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

apb7 commented Aug 3, 2018

seanlip left a comment

Choose a reason for hiding this comment

apb7 commented Aug 4, 2018

apb7 commented Aug 4, 2018

apb7 commented Jul 20, 2018 •

edited

Loading

codecov-io commented Jul 21, 2018 •

edited

Loading

seanlip Aug 3, 2018 •

edited

Loading

apb7 Aug 3, 2018 •

edited

Loading

apb7 Aug 3, 2018 •

edited

Loading