Run on e2e setup, test add to project flow, prepwork for Run On OpenShift tests #1057

benjaminapetersen · 2016-12-19T22:41:56Z

This PR is easiest to follow starting with: test/integration/features/user_adds_template_to_project.spec.js.

A few points:

test/integration/features is where I moved the actual test cases based on patterns I've seen in other protractor suites.
Individual tests are named via flows a user would take through the app
The /page-objects directory contains file that understand how to interact with individual pages in the web console
The /helpers dir contains some shared helpers
The /fixtures dir contains support data

If we can get this in, I will start working on the next step to support testing Run On Openshift, which will involve hacking the whitelist (see helpers/env.js) to install templates to a project that we can then use to test create via url.

RUN this suite alone:
$ grunt test-integration --suite=add-to-project
RUN the original suite that verified project creation:
$ grunt test-integration --suite=create-project
@jwforres @spadgett @rhamilto

benjaminapetersen · 2016-12-20T18:10:35Z

rebased

spadgett · 2016-12-21T16:19:06Z

test/integration/features/user_adds_template_to_project.spec.js

+  beforeEach(function() {
+    h.commonSetup();
+    h.login();
+    projectHelpers.deleteAllProjects();


I'm unclear why we're deleting all projects before every test. Will this delete everything I've created if I run the tests in my development environment?

It seems like the test that creates the project should delete it on tear down.

It should, but if a test fails protractor dies. It doesn't cleanup (and delete the project). So this is needed to ensure you start clean slate every time, otherwise you have to manually login as e2e-user and delete them by hand.

OK, so it only deletes e2e-user projects? I'd add a comment explaining.

dont forget to add the comment here like @spadgett requested

Got it, I'll add the comment in the projectHelpers file above deleteAllProjects

Oh, actually I already did there:

// All projects visible to the current user. // This function will click the 'delete' on every project that appears on the project list page. // Be careful about using this function if your test gives the e2e-user access // to internal projects such as openshift, or openshift-infra

spadgett · 2016-12-21T16:21:15Z

test/integration/features/user_adds_template_to_project.spec.js

+            var deploymentsPage = new DeploymentsPage(project);
+            deploymentsPage.visit();
+            var tableRows = element(by.repeater('replicationController in replicationControllersForDC'));
+            h.waitForElem(tableRows);


This testing style reads very well.

spadgett · 2016-12-21T16:24:19Z

test/integration/helpers/extend.js

+    _merge(target, source);
+  });
+  return target;
+};


Is there no lodash utility that does this?

Yup, both _.merge and _.extend would work. I think we haven't used Lodash in the tests yet, so I didn't require() it. We can start though and drop this helper.

Part of me wants to borrow the API from Backbone.js for this and do:

exports.FooPage = Page.extend({ // add methods... });

and eliminate the clunkiness of extend(FooPage.prototype, Page.prototype, { /* */ }), but that can prob come as a cleanup later.

Yeah I think I'd rather use _.extend (or even angular.extend) rather than rewriting it. I don't really see a reason not to include lodash in our test code if we need it.

replace extend with _.extend

spadgett · 2016-12-21T16:27:14Z

test/integration/helpers/inputs.js

+            if(!selected) {
+              return checkboxElem.click();
+            }
+          });


Most of the codebase uses indentation style:

return checkboxElem.isSelected().then(function(selected) { if(!selected) { return checkboxElem.click(); } });

I think we should start using that consistently.

Plug for PR #682 (eslint)! I'll update this.

spadgett · 2016-12-21T16:31:04Z

test/integration/helpers/project.js

@@ -61,6 +61,8 @@ exports.deleteAllProjects = function() {
    browser.wait(protractor.ExpectedConditions.elementToBeClickable(deleteButton), 2000);
    deleteButton.click();
    h.waitForPresence(".alert-success", "marked for deletion");
+    // dont click next until we know the modal disappeared
+    browser.sleep(1000);


Is it better to have a waitForRemoval or similar and wait for the DOM element to disappear?

Great idea, I'll add this.

create h.waitForRemvoal() helper

spadgett · 2016-12-21T16:35:23Z

test/integration/page-objects/catalog.js

+    var tabs = element(by.css('.nav-tabs'));
+    h.waitForElem(tabs);
+    return tabs;
+  },


Suggest a utility for clicking tabs that we can use for any page. We have a lot of tabs.

Hmm. I'm going to follow-up TODO for this one. I'm not sure if it should be a helper, or something built into the Page itself (like Page menu items) but tabs defined via constructor.

spadgett · 2016-12-21T16:38:04Z

test/integration/page-objects/page.js

+    },
+    clickLogout: function() {
+      return clickHelpMenuChild(by.id('iser-dropdown'), by.css('.dropdown.open .uib-dropdown-menu'), 0);
+    }


Can we find the item by the label? Doing it by index will break if we ever add items in the middle or reorder the menu.

find by label or cssContainingText

spadgett · 2016-12-21T16:38:57Z

test/integration/page-objects/page.js

+    },
+    // applications submenu
+    clickDeployments: function() {
+      return clickSideMenuChild(by.repeater('primaryItem in navItems'), 1, by.repeater('secondaryItem in secondarySection.items'), 0);


Same comment here: I'd really like to avoid selecting things by child index.

fix selector

spadgett · 2016-12-21T16:40:17Z

test/protractor.conf.js

@@ -172,10 +174,10 @@ exports.config = {
  // The timeout in milliseconds for each script run on the browser. This should
  // be longer than the maximum time your application needs to stabilize between
  // tasks.
-  allScriptsTimeout: 11000,
+  allScriptsTimeout: 30 * 1000,//11000,


So... 30 * 1000 != 11000, remove the comment?

I think // 30 seconds would be a better comment :)

Ha, oop, i left the old value in the comment as I was tinkering. Will eliminate, I think 30 seconds is obvious here.
Math win..?

Yeah I'm fine not adding the 30s comment because it's easy to forget to fix those when the code changes

spadgett · 2016-12-21T16:40:25Z

test/protractor.conf.js


  // How long to wait for a page to load.
-  getPageTimeout: 10000,
+  getPageTimeout: 30 * 1000, //10000,


Remove the incorrect comment

benjaminapetersen · 2016-12-21T17:38:18Z

General Q for @spadgett @jwforres going forward, since this model using PageObjects is emulating classical programming, and we are in Node.js, any concerns over using some ES6 syntax?

// classes
class Page {
  // etc
}

class FooPage extends Page {
  // etc
}

// fat arrows
doFoo.then((etc) => {
 // do things with etc
})

Since we aren't in the browser this might be appropriate. I'm not gonna change it for this PR, but going forward would be good to answer how we want to shape our tests.

benjaminapetersen · 2016-12-21T20:25:22Z

@spadgett second pass? other than the tabs comment, all should be fixed.

spadgett · 2016-12-21T21:16:36Z

test/integration/helpers/strings.js

+var _ = require('lodash');
+
+exports.startCase = function(str) {
+  return str && _.startCase(str) || '';


Can we just call _.startCase directly? It handles null / undefined values already (returning '')

Yup. I'll delete this.

spadgett · 2016-12-21T21:16:58Z

test/integration/helpers/inputs.js

+
+// example:
+//   unCheck(element(by.css('input[type="checkbox"]')))
+var unCheck = function(checkboxElem) {


nit: can we call it uncheck :)

spadgett · 2016-12-22T13:02:59Z

[test]

spadgett · 2016-12-22T13:26:20Z

1) User adds a template to a project after creating a new project using the "Import YAML/JSON" tab should process and create the objects in the template
  - Error: Not found
Wait timed out after 5085ms

benjaminapetersen · 2016-12-22T15:11:21Z

Interesting, gonna
[TEST]
again while I debug locally. Its struggling to find the 2 rows in the deployments table.
at /data/src/github.com/openshift/origin-web-console/test/integration/features/user_adds_template_to_project.spec.js:43:15
Will be curious to see if its flakey, no trouble finding these when I run on my machine.

benjaminapetersen · 2016-12-22T15:56:13Z

@spadgett hmmm. Curious if you have any thoughts. I'm wondering if there is latency in the test environment? I can run these locally fine. It fails when it gets to the end of the test & checks to see if the deployments are listed.

benjaminapetersen · 2016-12-22T19:04:15Z

Ok, passed once, removing an extra timeout & [TEST] one more time

benjaminapetersen · 2016-12-22T19:13:50Z

[TEST]
jshint lies.

benjaminapetersen · 2016-12-22T19:36:05Z

[TEST] with a shorter sleep hack

benjaminapetersen · 2017-01-23T20:00:48Z

[TEST]

benjaminapetersen · 2017-01-23T20:44:04Z

[TEST]

benjaminapetersen · 2017-01-23T21:04:02Z

@spadgett @jwforres @rhamilto yay, they run!!!!!
Ready for a review. Once these get a 👍 I can write the tests for the run-on stuff.

benjaminapetersen · 2017-01-24T22:34:13Z

[TEST]

benjaminapetersen · 2017-01-24T23:10:03Z

[TEST]

benjaminapetersen · 2017-01-25T01:29:01Z

@jwforres @spadgett should be ready for review. Since the PageObjects are OOP, I updated to using es6 classes for everything (and let, const, fat arrows, etc.). Since its all in node no concerns about running with the "new shiny" JS stuff. 😄

Thoughts?

benjaminapetersen · 2017-01-25T14:02:14Z

squashed & rebased.

jwforres · 2017-01-25T17:13:08Z

test/protractor.conf.js

@@ -89,7 +89,9 @@ exports.config = {
  // run.
  suites: {
    // smoke: 'spec/smoketests/*.js',
-    rest_api: 'integration/rest_api/*.js' // This suite of tests should only require a running master api, it should not require a node
+    'add-to-project': 'integration/features/user_adds_template_to_project.spec.js',
+    'create-project': 'integration/features/user_creates_project.spec.js' // This suite of tests should only require a running master api, it should not require a node


these feel a bit specific for suite names to me, i feel like the split is typically going to be tests that require a working node and tests that don't. I can't remember the format for what a suite name can point to, is it only a single regexp string, or can it also be an array of strings?

I think it can be an array of strings. I think we can do both, but its really nice (and saves a lot of dev time) if you can pluck out one test flow at a time. I'm totally open to this growing/shrinking/shifting as we add tests.

Yup, can use matchers + arrays or strings:

suites: { smoke: [ 'integration/features/userr_foo_bar.spec.js', 'integration/features/user_baz.spec.js' ], 'rest-api': [ 'integration/features/user/**/*.spec.js', // api specific match ], 'one-off': '', 'really-long-running-test': '' },

ok its fine for now but if we start getting a bunch of these we may want to organize them

jwforres · 2017-01-25T17:27:28Z

test/integration/page-objects/page.js

+
+  }
+  visit() {
+    logger.log('visiting url:',this.getUrl());


nit but can you add a space after the comma before this.getUrl

jwforres · 2017-01-25T17:28:58Z

test/integration/page-objects/overview.js

+  clickAddToProject() {
+    let button = element(by.cssContainingText('.project-action-btn', 'Add to project'));
+    h.waitForElem(button);
+    // browser.wait(protractor.ExpectedConditions.elementToBeClickable(button), 2000);


reason this is there and commented out?

I'll eliminate

jwforres · 2017-01-25T17:30:23Z

test/integration/page-objects/menus.js

+      return element(by.cssContainingText('a', 'Storage')).click();
+    },
+    clickMonitoring: () => {
+      return element(by.cssContainingText('a', 'MOnitoring')).click();


typo on Monitoring, should be lower case 'o'

jwforres · 2017-01-25T17:31:35Z

test/integration/page-objects/menus.js

+    clickOtherResources: () => {
+      return clickNestedMenuItem(by.cssContainingText('.dropdown-toggle', 'Resources'), by.cssContainingText('a', 'Other Resources'));
+    },
+    // rest


i wasnt really sure what this comment meant initially, could probably be more clear

I'll update to // the rest of the top lvl menu items. It may have been good to just nest these for clarity, ie leftNav.apps.clickDeployment(), leftNav.builds.clickPipelines(). I'm open.

jwforres · 2017-01-25T17:42:11Z

test/integration/page-objects/README.md

+- `return new SomeOtherPage(this.project)` when a function will cause a navigation action
+  - Include a `browser.sleep(500)` before the return if the new page may take some time to load
+- Encapsulate actions on the page as much as possible so test cases read simply
+- Use `helpser/matchers.js` to encapsulate repetitious `expect()` logic


typo, helpers

jwforres · 2017-01-25T17:45:00Z

test/integration/helpers/screenshot.js

+
+exports.take = function(savePath) {
+  return browser.takeScreenshot().then(function(png) {
+    var stream = fs.createWriteStream(savePath || '/tmp/origin-web-console-e2e-screenshot.png');


we should avoid taking screenshots during tests except when failures occur, can you add a comment in here that says as much. Screenshots are taken automatically today when protractor sees a failure so you need a good reason to explicitly take another screenshot. Obviously also fine to use this for debugging purposes while writing tests as long as the calls are taken out before its merged

I'll add a // for debugging only comment

jwforres · 2017-01-25T17:52:47Z

test/integration/helpers/logger.js

+
+// Use logger functions in place of console to ensure that log messages are wrapped
+// in a promise & resolved asyncronously.
+// NOTE: even if you don't use .then(), the promise queue is still synchromized in protractor.


typo, synchronized

jwforres · 2017-01-25T17:55:56Z

test/integration/helpers/env.js

+// - the browse catalog flow can be run w/o needing this, however
+exports.addNamespaceToCreateWhitelist = function(namespace) {
+  return browser.executeScript(function() {
+    window.CREATE_FROM_URL_WHITELIST.push(namespace);


isnt this window.OPENSHIFT_CONSTANTS.CREATE_FROM_URL_WHITELIST

yes, definitely.

jwforres · 2017-01-25T18:00:12Z

test/integration/helpers.js

+//  waitForElement(element(by.css('.foo')));  // success
+//  waitForElement(element.all(by.css('.foos')));  // fail, incorrect element.all
+var waitForElem = function(elem, timeout) {
+  return browser.wait(EC.presenceOf(elem), timeout || 5000, 'Element not found');


if this fails, what kind of error will you see, are you going to see the selector information somewhere to help track what failed?

Good call! I'll update to 'Element not found: ' + elem.locator().toString(). That will print out something like this:

- Error: Element not found: by.cssContainingText("a", "STUFF AND THINGS")

…t tests - Use es6 classes to add PageObjects to organize tests - Add h.waitForElem() and adjust h.waitForPresence() to allow for a callback fn - Add helpers/scroll - Add helpers/strings - Add README for pageobjects - Add base 'Page' PageObject for other pages to extend - Add createFromTemplate PageObject - Add deployments PageObject - Update helpers/matchers, set hLevel in expectHeading(), add expectPartialHeading - Add browser.sleep() to deleteAllProjects to ensure each project is removed - Move all e2e test flows under test/integration/features/ and name them based on user actions - Add xit to jshintrc - Cleanup user_adds_template_to_project.spec - Add numerous other helpers and page objects

openshift-bot · 2017-01-25T20:07:13Z

Evaluated for origin web console test up to 0c4ca98

openshift-bot · 2017-01-25T20:21:31Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/987/) (Base Commit: e202d9f)

jwforres · 2017-01-25T22:10:23Z

[merge]

benjaminapetersen · 2017-01-25T22:11:22Z

WOOHOO! my life now has meaning. 😃

openshift-bot · 2017-01-25T22:15:11Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/987/) (Base Commit: e202d9f)

openshift-bot · 2017-01-25T22:15:12Z

Evaluated for origin web console merge up to 0c4ca98

jhadvig · 2017-01-31T09:49:27Z

Looks great @benjaminapetersen
👍

benjaminapetersen requested a review from jwforres December 19, 2016 22:42

benjaminapetersen force-pushed the run-on-e2e-setup branch 2 times, most recently from 104dd2e to 24188ef Compare December 20, 2016 18:09

benjaminapetersen requested a review from spadgett December 21, 2016 16:14

spadgett reviewed Dec 21, 2016

View reviewed changes

spadgett self-assigned this Dec 21, 2016

benjaminapetersen force-pushed the run-on-e2e-setup branch 2 times, most recently from fe3e19f to d024745 Compare December 21, 2016 20:24

spadgett reviewed Dec 21, 2016

View reviewed changes

benjaminapetersen force-pushed the run-on-e2e-setup branch from d024745 to bd9f469 Compare December 21, 2016 21:43

benjaminapetersen force-pushed the run-on-e2e-setup branch from bd9f469 to 8b7c56c Compare December 22, 2016 15:20

benjaminapetersen force-pushed the run-on-e2e-setup branch from 3a2dfd0 to 0130d75 Compare December 22, 2016 18:57

benjaminapetersen force-pushed the run-on-e2e-setup branch from d73d32b to 387b142 Compare December 22, 2016 20:07

benjaminapetersen force-pushed the run-on-e2e-setup branch from 387b142 to 27fd707 Compare January 23, 2017 19:57

benjaminapetersen force-pushed the run-on-e2e-setup branch from 27fd707 to 05f1fe9 Compare January 23, 2017 20:43

benjaminapetersen force-pushed the run-on-e2e-setup branch from b7a3d0b to 0fa463e Compare January 24, 2017 23:09

benjaminapetersen force-pushed the run-on-e2e-setup branch from 0fa463e to 063fb0d Compare January 24, 2017 23:13

benjaminapetersen force-pushed the run-on-e2e-setup branch from 063fb0d to 4ead7da Compare January 25, 2017 13:46

benjaminapetersen force-pushed the run-on-e2e-setup branch from 4ead7da to 552d4b5 Compare January 25, 2017 14:54

jwforres requested changes Jan 25, 2017

View reviewed changes

benjaminapetersen force-pushed the run-on-e2e-setup branch from 552d4b5 to 0c4ca98 Compare January 25, 2017 19:55

jwforres approved these changes Jan 25, 2017

View reviewed changes

openshift-bot merged commit a2e9c6e into openshift:master Jan 25, 2017

benjaminapetersen deleted the run-on-e2e-setup branch January 25, 2017 22:20

Run on e2e setup, test add to project flow, prepwork for Run On OpenShift tests #1057

Run on e2e setup, test add to project flow, prepwork for Run On OpenShift tests #1057

Conversation

benjaminapetersen commented Dec 19, 2016 • edited by jwforres Loading

benjaminapetersen commented Dec 20, 2016

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

benjaminapetersen Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen Dec 21, 2016 • 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

benjaminapetersen commented Dec 21, 2016

benjaminapetersen commented Dec 21, 2016

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

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Jan 23, 2017

benjaminapetersen commented Jan 23, 2017

benjaminapetersen commented Jan 23, 2017

benjaminapetersen commented Jan 24, 2017

benjaminapetersen commented Jan 24, 2017

benjaminapetersen commented Jan 25, 2017

benjaminapetersen commented Jan 25, 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

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

openshift-bot commented Jan 25, 2017

benjaminapetersen commented Dec 19, 2016 •

edited by jwforres

Loading

benjaminapetersen Dec 21, 2016 •

edited

Loading

benjaminapetersen Dec 21, 2016 •

edited

Loading

benjaminapetersen Dec 21, 2016 •

edited

Loading

benjaminapetersen Dec 21, 2016 •

edited

Loading

openshift-bot commented Jan 25, 2017 •

edited

Loading