Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Functional/Integration tests #63

Merged
merged 4 commits into from
Oct 14, 2015
Merged

Functional/Integration tests #63

merged 4 commits into from
Oct 14, 2015

Conversation

teneightfive
Copy link
Contributor

User Story #103485730

As a developer,
I want the cashbook to be able to run integration/functional tests,
So that I know I haven't broken anything with my changes

@teneightfive teneightfive force-pushed the integration-tests branch 8 times, most recently from fd9d987 to 0f8930a Compare September 23, 2015 14:07
SteveMarshall
SteveMarshall reviewed Sep 25, 2015
View reviewed changes
test/features/pages/signin.js
var that = this;

this.get(function () {
that.fillForm(testUsername, testPassword, callback);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to have this just call this.signinWith(testUsername, testPassword, callback)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessary as siginWith is only calling fillForm anyway? This skips having to call another method which just calls the fillForm method anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I wonder if actually it'd be nicer as one function with default values for username and password? That way, the intent is much clearer and you're not spreading “sign in” functionality across two functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then you would end up with having to call the method with null values if you wanted to use the default creds: signin(null, null, callback).

Unless you flip the callback param to the front but that breaks the pattern being used elsewhere and by cucumber.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, god, yeah, of course it does. Roll on ES6, I guess? Leave it as-is, I suppose!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, would be much nicer with ES6. Was actually considering using coffee for some of this as it is a bit nicer for writing tests.

But assumed it might be better to not add too many more layers at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's leave it as-is; less layers == better.

@SteveMarshall
Copy link
Member

This is really good, @teneightfive! 🙌

Is it worth adding a test for something like:

As a user
I need to sign in before I can access the dashboard
So that I can't see anything I shouldn't

@teneightfive
Copy link
Contributor Author

@SteveMarshall sounds like it would be the same as the user_sigin test.

Maybe a case of just re-wording that if you're not happy with that wording?

@SteveMarshall
Copy link
Member

I think it's a different scenario, actually, or maybe in the tests for the dashboard (and other pages).

Something like:

Scenario: Haven't signed in, so can't do stuff
  Given I am not signed in
  When I go to the "Dashboard" page
  Then I should see "You are not logged in"

… or maybe Then I should see the login page?

@teneightfive
Copy link
Contributor Author

Yeah, that could be a different scenario in that case.

We need a separate story for writing out all the current scenarios too.

@teneightfive
Copy link
Contributor Author

retest this please

@teneightfive
Copy link
Contributor Author

@SteveMarshall just had a thought about this...shall we get this working with Jenkins before we merge?

@SteveMarshall
Copy link
Member

@teneightfive I think the way to get it “working with Jenkins” is going to be to get it working with a pure docker-compose setup, then have a make integration-tests type thing?

maxf
maxf reviewed Oct 6, 2015
View reviewed changes
test/features/user_signout.feature
Scenario: Successful sign out
Given I am signed in
When I sign out
Then I should see "Digital cashbook" as the page title
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cashbook was capitalised since

@SteveMarshall
Copy link
Member

@teneightfive @maxf One of you wanna fix the remaining bits on here?

@maxf
Copy link
Contributor

maxf commented Oct 9, 2015

@SteveMarshall fixed some remaining bits. Note that one test currently fails because of #104729954

SteveMarshall
SteveMarshall reviewed Oct 9, 2015
View reviewed changes
bower.json
"money-to-prisoners-common": "ministryofjustice/money-to-prisoners-common#0.3.0"
>>>>>>> origin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you wanted to commit this, @maxf?

@maxf
Copy link
Contributor

maxf commented Oct 9, 2015

retest this please

@SteveMarshall
Copy link
Member

LGTM; rebase and merge?

teneightfive and others added 4 commits October 14, 2015 12:32
@teneightfive
Add initial integration test setup
3798add 
Setup using cucumberjs to run gherkin style feature tests
@teneightfive
Switch to using webdriverio over selenium-webdriver
658c660
update tests to reflect changes in app
4eab16d
avoid installing unnecessary packages
e67e493 
Don't install browser-sync and other dependent packages
on non-local environments, as those packages are only
useful on developer machines
[updates #104986742]
maxf added a commit that referenced this pull request Oct 14, 2015
@maxf
Merge pull request #63 from ministryofjustice/integration-tests
9e525fa 
Functional/Integration tests
@maxf maxf merged commit 9e525fa into master Oct 14, 2015
@teneightfive
Copy link
Contributor Author

👍

@SteveMarshall SteveMarshall deleted the integration-tests branch October 27, 2015 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
review me
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@teneightfive @SteveMarshall @maxf