-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create from URL e2e tests #1195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few small nits since I'm fairly familiar with all this. Def looking forward to having out test suite grow.
catalogPage | ||
.processImageStream(JSON.stringify(centosImageStream)) | ||
.then(function() { | ||
// verify we have the nodejs image stream loaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd kind of like to be able to get rid of this comment and have something in code like:
expect(tableRowFor('nodejs')).toBe(true);
But we can prob add that helper later as I bet we would want a few table helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a TODO so we're consistent with user_adds_template_to_project.spec.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
let namespaceForFixtures = "template-dumpster"; | ||
|
||
let setupEnv = function() { | ||
// add namespace to create whitelist for template and image stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let name = 'nodejs-edited'; | ||
let sourceURI = 'https://github.com/openshift/nodejs-ex.git-edited'; | ||
let sourceRef = 'master-edited'; | ||
let contextDir = '/-edited'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can update the qs
var to get rid of the +
with a template literal and ${placeholders}
:
let qs = `?imageStream=nodejs&imageTag=4&name=${name}&sourceURI=${sourceURI}&sourceRef=${sourceRef}&contextDir=${contextDir}&namespace=${namespaceForFixtures}`;
let uri = `project/${project.name}create/fromimage${qs}`;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
it('should load the image stream in to a newly created project', function(){ | ||
let createFromURLPage = new CreateFromURLPage(); | ||
createFromURLPage.visit(qs); | ||
// NOTE: if the tabs are on the page because there are existing projects, click the 'Create New Project' tab first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop this comment since we don't have the if
statement here anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
expect(nameInput.getAttribute('value')).toEqual(name); | ||
let sourceURIInput = element(by.model('buildConfig.sourceUrl')); | ||
expect(sourceURIInput.getAttribute('value')).toEqual(sourceURI); | ||
// click "Show advanced..." link to reveal hidden fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be pushed up into the page object as a method .clickShowAdvanced()
& eliminate the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
it('should load the template in to a newly created project', function() { | ||
let createFromURLPage = new CreateFromURLPage(); | ||
createFromURLPage.visit(qs); | ||
// NOTE: if the tabs are on the page because there are existing projects, click the 'Create New Project' tab first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, i think we changed this to not use the if/else
so can delete the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -42,24 +38,42 @@ describe('User adds a template to a project', function() { | |||
// verify we have the 2 deployments in the template | |||
var deploymentsPage = new DeploymentsPage(project); | |||
deploymentsPage.visit(); | |||
expect(element(by.cssContainingText('td', 'mongodb')).isPresent()).toBe(true); | |||
expect(element(by.cssContainingText('td', 'nodejs-mongodb-example')).isPresent()).toBe(true); | |||
expect(element(by.cssContainingText('td', 'mongodb')).isPresent()).toBe(true); // TODO: use fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO's should be obvious what is needed for someone coming else looking at this later. Also fixture isn't doesn't seem like the right word here, shouldn't it be a helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment means to eventually use the fixture (json) and pluck out the names, rather than hard-coding mongodb
, nodejs-mongodb-example
.
Is on my list to add some table helpers as well.
const path = require('path'); | ||
// to use: a template literal is probably the easiest way to provide JS to inject into the | ||
// extensions.js file. Please wrap in IIFE as addExtension() may be called multiple times & | ||
// the file contents will be concatonated: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: concatenated
// `; | ||
// adding to the extension.js file: | ||
// addExtension(jsToWrite) | ||
exports.addExtension = function(fileContents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont ever see this cleaning up after itself, are the tests leaving .tmp/extensions.js in a modified state? It should at least make a best effort to clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That whole directory gets blown away every time we build. running the tests over and over will generate all new files. Still think its necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so long as there is a hard refresh which will pick up a cleaned up extensions.js file, we could clean it up. But without a hard refresh updates won't change in the browser. Hmmm.
I'm more worried that individual tests are affecting other tests.
…On Tue, Jan 31, 2017 at 4:05 PM, Ben Petersen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/integration/helpers/extensions.js
<#1195>:
> +'use strict';
+
+const fs = require('fs');
+const path = require('path');
+// to use: a template literal is probably the easiest way to provide JS to inject into the
+// extensions.js file. Please wrap in IIFE as addExtension() may be called multiple times &
+// the file contents will be concatonated:
+// var jsToWrite = `
+// (function() {
+// // createFromURL test file ${testTime} // (can use interpolation))
+// window.OPENSHIFT_CONSTANTS.CREATE_FROM_URL_WHITELIST = ['openshift', 'template-dumpster'];
+// })();
+// `;
+// adding to the extension.js file:
+// addExtension(jsToWrite)
+exports.addExtension = function(fileContents) {
That whole directory gets blown away every time we build. running the
tests over and over will generate all new files. Still think its necessary?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1195>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/ABZk7c7Aaa5ca7tM6taga0yveGwDeKj4ks5rX6IagaJpZM4Lx_zA>
.
|
can we squash the commits in the branch a bit, could squash the last 3 together |
Add resetExtensions() fn to helpers/extensions.js
Done. |
[merge] |
Evaluated for origin web console merge up to 09ea11d |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1022/) (Base Commit: 35f0730) |
yay! |
Huge assist from @benjaminapetersen