-
Notifications
You must be signed in to change notification settings - Fork 39
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
Changes from 15 commits
8f10f47
7728d4c
06f1d3f
adbdbff
7313017
4f2d582
cd2947f
227b7ba
2091748
ddaa0c9
ed8659f
eea027d
0aa4bbd
514e5b6
93b096d
41e49eb
7b2f3b1
781ed81
6a20fbd
6bfd5c8
bc75a16
669d71a
480c3d6
d4e63d3
a399056
80c272c
bc0e632
c12ce72
a751d6e
c6bb595
28208e4
ef7d697
842875d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// 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); | ||
}); | ||
}); |
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A good quote from the docs:
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'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,22 +2,45 @@ import { module } from 'qunit'; | |
import Ember from 'ember'; | ||
import startApp from '../helpers/start-app'; | ||
import destroyApp from '../helpers/destroy-app'; | ||
import FactoryGuy, { manualSetup } from 'ember-data-factory-guy'; | ||
import config from 'ember-get-config'; | ||
import FakeServer, { stubRequest } from 'ember-cli-fake-server'; | ||
|
||
const { RSVP: { Promise } } = Ember; | ||
|
||
export default function(name, options = {}) { | ||
module(name, { | ||
beforeEach() { | ||
this.application = startApp(); | ||
|
||
if (options.beforeEach) { | ||
return options.beforeEach.apply(this, arguments); | ||
} | ||
this.application = startApp(); | ||
FakeServer.start(); | ||
manualSetup(this.application.__container__); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General comment for this PR: is this necessary? From docs:
See also the factory guy setup for comparison in case it sparks some ideas: |
||
const url = config.OSF.apiUrl; | ||
const provider = FactoryGuy.build('preprint-provider'); | ||
stubRequest('get', url + '/v2/users/me', (request) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks helpful! I ran the tests in chrome with the network inspector open, and noticed that the page still sends a request to google analytics every time tests run. This may indicate one more thing that needs stubbing. Thoughts? |
||
request.unauthorized({}); | ||
}); | ||
stubRequest('get', url + '/v2/preprint_providers', (request) => { | ||
request.ok({data: [{ | ||
attributes: provider, | ||
type: "preprint_providers", | ||
id: "osf" | ||
}]}); | ||
}); | ||
stubRequest('get', url + '/v2/preprint_providers/osf', (request) => { | ||
request.ok({data: { | ||
attributes: provider, | ||
type: "preprint_providers", | ||
id: "osf" | ||
}}); | ||
}); | ||
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)); | ||
} | ||
}); | ||
} |
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')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add message? |
||
|
||
contributor.users.identifiers.push('https://staging.osf.io/cool'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this selector be simplified to |
||
assert.equal(this.$().text().trim(), contributorModel.get('users.fullName')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add message? |
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { moduleForComponent, test } from 'ember-qunit'; | ||
|
||
moduleForComponent('subject-picker', 'Integration | Component | subject picker', { | ||
integration: true | ||
}); | ||
|
||
test('it renders', function(assert) { | ||
|
||
//TODO: looks like author-link tests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
assert.ok(true); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,98 @@ | ||
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See questions above about computeds/ use of Also, if we are intending to test multiple different scenarios, I generally recommend doing each in a separate |
||
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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Eg: |
||
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.trim()); | ||
|
||
//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.trim()); | ||
|
||
//Test less than 350 doesn't use description (uses node.description) | ||
|
||
ctrl.set('expandedAbstract', true); | ||
// expandedAbstract logic has been moved to the template, making the expanded tests unnescessary here. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per this note: does this mean that any code after the last assertion can be removed? |
||
}); | ||
}); |
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.
🦑 Note: this is fine for local testing, but should be changed to a real ember-osf release before this gets merged. (blocking change)