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.

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

[EOSF-594] Add example controller unit tests and component integration tests #293

Merged
merged 33 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8f10f47
add facotry-guy, mockjax and acceptance test
Feb 21, 2017
7728d4c
Fix module for acceptance, play with content test some more
Feb 27, 2017
06f1d3f
Flesh out content controller tests
Feb 28, 2017
adbdbff
New component tests:
Mar 1, 2017
7313017
Adjust author-link tests
Mar 3, 2017
4f2d582
Merge branch 'develop' of http:/CenterForOpenScience/embe…
Mar 3, 2017
cd2947f
Clean up tests and remove unused code
Mar 10, 2017
227b7ba
Make sure bower has Faker
Mar 14, 2017
2091748
Update yarn with pinned ember-osf commit
hmoco Mar 20, 2017
ddaa0c9
Merge branch 'feature/tests' of http:/CenterForOpenScienc…
Mar 21, 2017
ed8659f
Update packages, add stubbing to requests in acceptance tests
Mar 21, 2017
eea027d
Merge branch 'develop' of http:/CenterForOpenScience/embe…
Mar 21, 2017
0aa4bbd
Merge branch 'feature/tests' of http:/CenterForOpenScienc…
Mar 21, 2017
514e5b6
Remove obsolete tests
Mar 21, 2017
93b096d
Update module-for-acceptance.js
hmoco Mar 22, 2017
41e49eb
skip do nothing test, start discover page setup
Mar 27, 2017
7b2f3b1
unsure on how to ensure data is available fully prior to hitting andThen
Mar 27, 2017
781ed81
Make separates and joins of copyright providers use ', ' rather than ','
Mar 27, 2017
6a20fbd
Play around some more with acceptance/runs
Mar 28, 2017
6bfd5c8
remove unused tests (acceptance) and clean up/separate other tests
Mar 29, 2017
bc75a16
Merge branch 'develop' of http:/CenterForOpenScience/embe…
hmoco Apr 6, 2017
669d71a
Merge branch 'feature/tests' of http:/CenterForOpenScienc…
hmoco Apr 6, 2017
480c3d6
Attempt to recreate yarn file
hmoco Apr 7, 2017
d4e63d3
Attempt to fix lockfile conflicts
hmoco Apr 7, 2017
a399056
Merge branch 'feature/tests' of http:/CenterForOpenScienc…
hmoco Apr 24, 2017
80c272c
Clean up packages, pin ember data facotry guy version
hmoco Apr 24, 2017
bc0e632
Merge branch 'develop' of http:/CenterForOpenScience/embe…
hmoco Apr 26, 2017
c12ce72
Add messages to test failures
hmoco Apr 26, 2017
a751d6e
Make split and joins consistent across copyrightHolders and test for it
hmoco Apr 27, 2017
c6bb595
Distinguish async behavior on discardbasics, test for copyright holde…
hmoco Apr 27, 2017
28208e4
Revert changes made to submit conroller made in c6bb5953137687cd2cd0b…
jamescdavis Apr 28, 2017
ef7d697
Merge branch 'develop' of http:/CenterForOpenScience/embe…
hmoco Apr 28, 2017
842875d
Move filedownloadUrl test to its component
hmoco Apr 28, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion bower.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
"toastr": "^2.1.2",
"hint.css": "^2.3.2",
"jquery.tagsinput": "^1.3.6",
"loaders.css": "^0.1.2"
"loaders.css": "^0.1.2",
"jquery-mockjax": "~2.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

jquery-mockjax sounds like something used only for testing. Could I suggest moving it to devDependencies instead of dependencies?

See: http://stackoverflow.com/questions/19339227/bower-and-devdependencies-vs-dependencies

},
"resolutions": {
"jquery": "~2.2.4"
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@
"ember-collection": "1.0.0-alpha.6",
"ember-cp-validations": "^2.9.4",
"ember-data": "^2.7.0",
"ember-data-factory-guy": "^2.11.6",
"ember-export-application-global": "^1.0.5",
"ember-faker": "^1.1.1",
"ember-font-awesome": "2.1.1",
"ember-get-config": "0.0.4",
"ember-i18n": "4.3.1",
Expand Down
19 changes: 19 additions & 0 deletions tests/acceptance/content-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { test } from 'qunit';
import moduleForAcceptance from 'preprint-service/tests/helpers/module-for-acceptance';
// import FactoryGuy from 'ember-data-factory-guy';

moduleForAcceptance('Acceptance | content');

test('visiting /content', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test does not appear to do anything.

Rather than comment out broken functional code, consider using skip() in place of test(). This will help us track problematic tests for followup. We try to avoid committing comment blocks when possible.

// TODO: Problems getting preprint content page. url = '/' completely breaks testem
let container = this.application.__container__;
// let preprint = FactoryGuy.make('preprint');
// let url = '/' + preprint.get('id');
// url = '/'
// visit(url);

andThen(() => {
assert.ok(container);
// assert.equal(currentURL(), url);
});
});
29 changes: 29 additions & 0 deletions tests/acceptance/discover-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { test } from 'qunit';
import moduleForAcceptance from 'preprint-service/tests/helpers/module-for-acceptance';

moduleForAcceptance('Acceptance | discover');

test('visiting /discover', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On the one hand it's nice we test these things. On the other hand, I'm fairly sure there's a way for no-value query params to simply be omitted from the URL.

Is ?provider=&subject= a design behavior we want to test for, or a cosmetic bug?

visit('preprints/discover');
andThen(() => assert.equal(currentURL(), '/preprints/discover?provider=&subject='));
});

test('visit discover with queryParams', function(assert) {
let container = this.application.__container__;

visit('preprints/discover?provider=OSF&subject=Business');

andThen(() => {
let controller = container.lookup('controller:discover');
Copy link
Contributor

Choose a reason for hiding this comment

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

A good quote from the docs:

Acceptance tests are used to test user interaction and application flow. The tests interact with the application in the same ways that a user would, by doing things like filling out form fields and clicking buttons...

In general, it seems like the acceptance test should focus on what the result looks like to the user, without knowing how the internals (the controller) are implemented. Perhaps the things that check controller behavior would be better suited to, say, a unit test?

To make the difference actionable for this PR- one thing we could do is to check whether the desired checkboxes are correctly selected. That's a user-facing behavior related to the query params, and a good candidate for an acceptance test.

let providers = controller.activeFilters.providers;
let subjects = controller.activeFilters.subjects;

assert.ok(providers.indexOf('OSF') !== -1);
assert.equal(providers.length, 1);

assert.ok(subjects.indexOf('Business') !== -1);
assert.equal(subjects.length, 1);

assert.equal(currentURL(), 'preprints/discover?provider=OSF&subject=Business');
});
});
16 changes: 9 additions & 7 deletions tests/helpers/module-for-acceptance.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,24 @@ import { module } from 'qunit';
import Ember from 'ember';
import startApp from '../helpers/start-app';
import destroyApp from '../helpers/destroy-app';
import { manualSetup } from 'ember-data-factory-guy';

const { RSVP: { Promise } } = Ember;

export default function(name, options = {}) {
module(name, {
beforeEach() {
this.application = startApp();
this.application = startApp();
manualSetup(this.application.__container__);
Copy link
Contributor

Choose a reason for hiding this comment

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

General comment for this PR: is this necessary?

From docs:

By default, you only need to call manualSetup(this.container) in unit/component tests

See also the factory guy setup for comparison in case it sparks some ideas:
https:/danielspaniel/ember-data-factory-guy/blob/master/tests/helpers/module-for-acceptance.js


if (options.beforeEach) {
return options.beforeEach.apply(this, arguments);
}
if (options.beforeEach) {
return options.beforeEach.apply(this, arguments);
}
},

afterEach() {
let afterEach = options.afterEach && options.afterEach.apply(this, arguments);
return Promise.resolve(afterEach).then(() => destroyApp(this.application));
// FakeServer.stop();
let afterEach = options.afterEach && options.afterEach.apply(this, arguments);
return Promise.resolve(afterEach).then(() => destroyApp(this.application));
}
});
}
33 changes: 33 additions & 0 deletions tests/integration/components/author-link-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import Ember from 'ember';
import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';
import FactoryGuy, { manualSetup } from 'ember-data-factory-guy';

moduleForComponent('author-link', 'Integration | Component | author link', {
integration: true
});

test('renders links and non-links', function(assert) {

// Set any properties with this.set('myProperty', 'value');
// Handle any actions with this.on('myAction', function(val) { ... });
manualSetup(this.container);
let contributorModel = FactoryGuy.make('contributor');
// Problem here is that author link expects a share search-result contributor,
// not a store instance of a contributor and its user(s).
let contributor = {users: {identifiers: []}};
contributor.users.name = contributorModel.get('users.fullName');
contributor = Ember.merge(contributor, contributorModel.serialize().data.attributes);
this.set('contributor', contributor);

this.render(hbs`{{author-link contributor=contributor}}`);
assert.ok(!this.$().has('a').length);
assert.equal(this.$().text().trim(), contributorModel.get('users.fullName'));
Copy link
Member

Choose a reason for hiding this comment

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

add message?


contributor.users.identifiers.push('https://staging.osf.io/cool');
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that the component is being rendered twice.

Just to ensure tests are isolated, might I suggest splitting this into two tests? (the common setup parts could be moved into a beforeEach of course)

this.set('contributor', contributor);

this.render(hbs`{{author-link contributor=contributor}}`);
assert.ok(this.$().has('a').length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this selector be simplified to this.$.('a')?

assert.equal(this.$().text().trim(), contributorModel.get('users.fullName'));
Copy link
Member

Choose a reason for hiding this comment

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

add message?

});
16 changes: 16 additions & 0 deletions tests/integration/components/subject-picker-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

moduleForComponent('subject-picker', 'Integration | Component | subject picker', {
integration: true
});

test('it renders', function(assert) {

//TODO: looks like author-link tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this test is marked "TODO", I'd suggest either implementing it, or removing it, before merge.


this.render(hbs`{{subject-picker}}`);

assert.equal(this.$().text().trim(), '');

});
103 changes: 99 additions & 4 deletions tests/unit/controllers/content-test.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,107 @@
import Ember from 'ember';
import { moduleFor, test } from 'ember-qunit';
import FactoryGuy, { manualSetup } from 'ember-data-factory-guy';


moduleFor('controller:content', 'Unit | Controller | content', {
// Specify the other units that are required for this test.
needs: ['service:metrics', 'service:theme']
needs: ['service:metrics', 'service:theme', 'model:file', 'model:preprint',
'model:preprint-provider', 'model:node', 'model:license',
'model:user', 'model:citation', 'model:draft-registration',
'model:contributor', 'model:comment', 'model:institution',
'model:registration', 'model:file-provider', 'model:log',
'model:node-link', 'model:wiki'],
beforeEach: function() {
manualSetup(this.container);
}
});

// Replace this with your real tests.
test('it exists', function(assert) {
let controller = this.subject();
assert.ok(controller);
let controller = this.subject();
assert.ok(controller);
});

test('fullLicenseText computed property', function(assert) {
let ctrl = this.subject();
let preprint = FactoryGuy.make('preprint');
let license = preprint.get('license');
ctrl.set('model', preprint);
license.set('text', 'On {{year}}, for {{copyrightHolders}}');

preprint.set('licenseRecord', {
year: '2001',
copyright_holders: ['Henrique', 'Someone Else']
});
ctrl.notifyPropertyChange('model.license');
assert.equal(ctrl.get('fullLicenseText'), 'On 2001, for Henrique,Someone Else');
Copy link
Contributor

Choose a reason for hiding this comment

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

As a human, I'm a little surprised there's no space between the two names. Does this test reveal a possible cosmetic bug?

If so, we may want to fix it rather than hardcoding the old behavior as the desired one. I'm conscious of scope creep, but also of the importance of a test suite that reflects how the application should work.


preprint.set('licenseRecord', {
year: '',
copyright_holders: []
});
ctrl.notifyPropertyChange('model.license');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why this is necessary? Shouldn't computeds automatically recalculate when the property changes?

assert.equal(ctrl.get('fullLicenseText'), 'On , for ');

preprint.set('licenseRecord', {
year: '{{year}}',
copyright_holders: ['{{copyrightHolders}}']
});
ctrl.notifyPropertyChange('model.license');
Copy link
Contributor

Choose a reason for hiding this comment

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

See questions above about computeds/ use of notifyPropertyChange.

Also, if we are intending to test multiple different scenarios, I generally recommend doing each in a separate test to ensure that the behaviors are isolated. It does not appear that the three scenarios are interdependent, though I could be missing something.

assert.equal(ctrl.get('fullLicenseText'), 'On {{year}}, for {{copyrightHolders}}');
});

test('useShortenedDescription computed property', function(assert) {
let ctrl = this.subject();
let node = FactoryGuy.make('node');
ctrl.set('node', node);
Ember.run(() => {
Copy link
Contributor

@abought abought Mar 23, 2017

Choose a reason for hiding this comment

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

  1. In general, whenever you put code in a run loop, you want to make sure it actually ran before the test ended. One very easy safeguard is to use assert.expect(99) at the very top of the test (outside the run loop) to tell it how many assertions you thought would run. (this has saved me a few times when testing async behavior)

  2. Does this need to be in Ember.run? If not, item 1 might not be necessary. ;)

  3. Realistically, there are three separate scenarios that could each be a separate test(). If we shorten this to provide each description when the node is first created, I think we can do away with the .run and the notifyPropertyChange altogether.

Eg: let user = make('node', {description: 's'.repeat(350)});

node.set('description', 'string'.repeat(100).slice(0, 351));
ctrl.notifyPropertyChange('node.description');
assert.ok(ctrl.get('useShortenedDescription'));

node.set('description', 'string'.repeat(100).slice(0, 350));
ctrl.notifyPropertyChange('node.description');
assert.ok(!ctrl.get('useShortenedDescription'));

node.set('description', 'string'.repeat(100).slice(0, 349));
ctrl.notifyPropertyChange('node.description');
assert.ok(!ctrl.get('useShortenedDescription'));
});
});

test('description computed property', function(assert) {
Ember.run(() => {
let ctrl = this.subject();
let node = FactoryGuy.make('node');
ctrl.set('node', node);
ctrl.set('expandedAbstract', false);

//Test cut at 350 characters
let description = 'string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string str';
let notExpanded = description.slice(0, 350) + '...';
node.set('description', description);
ctrl.notifyPropertyChange('description');
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned elsewhere, not sure this part is needed?

Also, each scenario should be its own test to ensure isolation.

assert.equal(ctrl.get('description'), notExpanded);

//Test cut at less than 350 characters to not cut in middle of word
description = 'string stringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstring';
notExpanded = 'string ...';
node.set('description', description);
ctrl.notifyPropertyChange('description');
assert.equal(ctrl.get('description'), notExpanded);

//Test less than 350 doesn't use description (uses node.description)

ctrl.set('expandedAbstract', true);

description = 'string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string string str';
node.set('description', description);
ctrl.notifyPropertyChange('description');
assert.equal(ctrl.get('description'), description);

description = 'string stringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstring';
node.set('description', description);
ctrl.notifyPropertyChange('description');
assert.equal(ctrl.get('description'), description);
});
});
11 changes: 11 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2058,6 +2058,13 @@ ember-cp-validations@^2.9.4:
exists-sync "0.0.3"
walk-sync "^0.2.0"

ember-data-factory-guy@^2.11.6:
version "2.11.6"
resolved "https://registry.yarnpkg.com/ember-data-factory-guy/-/ember-data-factory-guy-2.11.6.tgz#c4b2170f90ee7c3897c162f1fc5579a7e287919d"
dependencies:
broccoli-funnel "^1.0.1"
broccoli-merge-trees "^1.1.1"

[email protected]:
version "0.1.7"
resolved "https://registry.yarnpkg.com/ember-data-has-many-query/-/ember-data-has-many-query-0.1.7.tgz#f34cb7a3abf25c94a6265e2d072f3844e8f23c4d"
Expand Down Expand Up @@ -2098,6 +2105,10 @@ ember-export-application-global@^1.0.5:
dependencies:
ember-cli-babel "^5.1.10"

ember-faker@^1.1.1:
version "1.1.1"
resolved "https://registry.yarnpkg.com/ember-faker/-/ember-faker-1.1.1.tgz#90ca83edef385d8f43bed3ceaed263b59f68773c"

ember-factory-for-polyfill@^1.1.0:
version "1.1.1"
resolved "https://registry.yarnpkg.com/ember-factory-for-polyfill/-/ember-factory-for-polyfill-1.1.1.tgz#c1124d541a058baaa6681d9611340c16f0baf660"
Expand Down