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

hmoco · 2017-03-03T19:16:11Z

This PR has been diminished in scope due to problems found with acceptance testing. Ticket previously tracking this PR is https://openscience.atlassian.net/browse/EOSF-510

Ticket

https://openscience.atlassian.net/browse/EOSF-594

Purpose

Improve test coverage, with integration and controller tests, to allow simpler future tests writing.

Changes

Use of ember-osf factories

…r-preprints into feature/tests

binoculars · 2017-03-14T16:51:54Z

@hmoco tests are failing, can you fix?

abought · 2017-03-14T18:03:23Z

bower.json

@@ -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"


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

…e/ember-preprints into feature/tests

…r-preprints into feature/tests

…e/ember-preprints into feature/tests

abought

It's really good to see a start on testing. I can see some of the scaffolding here being very useful going forward.

That said, I do have a few suggestions on tests, eg making sure the acceptance tests focus on user facing behavior, and also possible ways to make the unit tests more concise / independent. Hopefully these will act as examples for future work.

In a few cases, it seems that your tests may have caught minor cosmetic bugs? Helping already!

abought · 2017-03-23T18:44:20Z

package.json

 "ember-font-awesome": "2.1.1",
 "ember-get-config": "0.0.4",
 "ember-i18n": "4.3.1",
 "ember-link-action": "0.0.35",
 "ember-load-initializers": "^0.5.1",
 "ember-metrics": "0.6.3",
- "ember-osf": "0.3.0",
+ "ember-osf": "https:/CenterForOpenScience/ember-osf.git#014fa91a442d4f2b8afe2adf0463acc1e3723ad3",


🦑 Note: this is fine for local testing, but should be changed to a real ember-osf release before this gets merged. (blocking change)

abought · 2017-03-23T18:46:15Z

tests/acceptance/content-test.js

+
+moduleForAcceptance('Acceptance | content');
+
+test('visiting /content', function(assert) {


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.

abought · 2017-03-23T18:54:02Z

tests/acceptance/discover-test.js

+
+moduleForAcceptance('Acceptance | discover');
+
+test('visiting /discover', function(assert) {


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?

abought · 2017-03-23T19:31:56Z

tests/acceptance/discover-test.js

+ visit('preprints/discover?provider=OSF&subject=Business');
+
+ andThen(() => {
+ let controller = container.lookup('controller:discover');


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.

abought · 2017-03-23T19:33:40Z

tests/helpers/module-for-acceptance.js

- }
+ this.application = startApp();
+ FakeServer.start();
+ manualSetup(this.application.__container__);


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

abought · 2017-03-23T19:47:15Z

tests/unit/controllers/content-test.js

+ year: '{{year}}',
+ copyright_holders: ['{{copyrightHolders}}']
+ });
+ ctrl.notifyPropertyChange('model.license');


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.

abought · 2017-03-23T19:50:31Z

tests/unit/controllers/content-test.js

+ let ctrl = this.subject();
+ let node = FactoryGuy.make('node');
+ ctrl.set('node', node);
+ Ember.run(() => {


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)

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

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)});

abought · 2017-03-23T19:57:49Z

tests/unit/controllers/content-test.js

+ 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');


As mentioned elsewhere, not sure this part is needed?

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

abought · 2017-03-23T20:09:17Z

tests/unit/controllers/content-test.js

+ //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.


Per this note: does this mean that any code after the last assertion can be removed?

abought · 2017-03-23T20:10:51Z

tests/helpers/module-for-acceptance.js

+ manualSetup(this.application.__container__);
+ const url = config.OSF.apiUrl;
+ const provider = FactoryGuy.build('preprint-provider');
+ stubRequest('get', url + '/v2/users/me', (request) => {


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?

…r-preprints into feature/tests

…e/ember-preprints into feature/tests

jamescdavis

This is a great start on improving test coverage. My only reservations are to make sure that even minor formatting changes are mentioned in Side Effects and that I recommend adding messages when there is more than one assertion in a test to make it clear why a test is failing. In general, I think messages are useful, even when there is only one assertion, and even if the assertion is somewhat obvious. I always think it's better to give explicit feedback when it comes to test failures.

jamescdavis · 2017-04-25T14:32:06Z

app/controllers/content/index.js

@@ -137,7 +137,7 @@ export default Ember.Controller.extend(Analytics, {
 let text = this.get('model.license.text');
 if (text) {
 text = text.replace(/({{year}})/g, this.get('model.licenseRecord').year || '');
- text = text.replace(/({{copyrightHolders}})/g, this.get('model.licenseRecord').copyright_holders ? this.get('model.licenseRecord').copyright_holders.join(',') : false || '');
+ text = text.replace(/({{copyrightHolders}})/g, this.get('model.licenseRecord').copyright_holders ? this.get('model.licenseRecord').copyright_holders.join(', ') : false || '');


Though this change is minor, it'd be good to mention it in Side Effects just to bring it to attention of QA and others.

jamescdavis · 2017-04-25T14:49:56Z

tests/integration/components/author-link-test.js

+ contributor = Ember.merge(contributor, contributorModel.serialize().data.attributes);
+ this.set('contributor', contributor);
+ this.render(hbs`{{author-link contributor=contributor}}`);
+ assert.ok(!this.$('a').length);


add message?

jamescdavis · 2017-04-25T14:50:42Z

tests/integration/components/author-link-test.js

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


add message?

jamescdavis · 2017-04-25T14:50:52Z

tests/integration/components/author-link-test.js

+ this.set('contributor', contributor);
+
+ this.render(hbs`{{author-link contributor=contributor}}`);
+ assert.ok(this.$('a').length);


add message?

jamescdavis · 2017-04-25T14:51:19Z

tests/integration/components/author-link-test.js

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


add message?

jamescdavis · 2017-04-25T14:54:06Z

tests/unit/controllers/content/index-test.js

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


add message?

jamescdavis · 2017-04-25T14:54:12Z

tests/unit/controllers/content/index-test.js

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


add message?

jamescdavis · 2017-04-25T14:54:44Z

tests/unit/controllers/content/index-test.js

+ 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);
+ assert.equal(ctrl.get('description'), notExpanded.trim());


add message?

jamescdavis · 2017-04-25T14:54:49Z

tests/unit/controllers/content/index-test.js

+ description = 'string stringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstringstring';
+ notExpanded = 'string ';
+ node.set('description', description);
+ assert.equal(ctrl.get('description'), notExpanded.trim());


add message?

jamescdavis · 2017-04-25T14:59:47Z

app/controllers/submit.js

@@ -304,7 +304,7 @@ export default Ember.Controller.extend(Analytics, BasicsValidations, NodeActions
 let license = this.get('model.license');
 return {
 year: record ? record.year : null,
- copyrightHolders: record && record.copyright_holders ? record.copyright_holders.join(',') : null,
+ copyrightHolders: record && record.copyright_holders ? record.copyright_holders.join(', ') : null,


SAA about mentioning this in Side Effects.

binoculars · 2017-04-25T15:51:52Z

Probably should revert changes in controllers, as I've made major updates in #319

jamescdavis · 2017-04-25T16:03:39Z

Indeed. It looks like #319 should probably be merged first? There will likely be several merge conflicts.

…r-preprints into feature/tests

jamescdavis

Need to add a submit controller test for multiple copyright holders.

jamescdavis · 2017-04-27T15:46:09Z

app/controllers/submit.js

@@ -684,7 +684,7 @@ export default Ember.Controller.extend(Analytics, BasicsValidations, NodeActions
 this.set('basicsLicense', {
 licenseType: license || this.get('availableLicenses').toArray()[0],
 year: this.get('model.licenseRecord') ? this.get('model.licenseRecord').year : date.getUTCFullYear().toString(),
- copyrightHolders: this.get('model.licenseRecord') ? this.get('model.licenseRecord').copyright_holders.join(',') : ''
+ copyrightHolders: this.get('model.licenseRecord') ? this.get('model.licenseRecord').copyright_holders.join(', ') : ''


Need to add a test for this.

…rs join

jamescdavis

Recommend using the wait() test helper instead to test asynchronous parts of the discardBasics action.

…) test helper instead Upgrading to ember-cli-qunit 3 was required due to bug in v2 that prevents use of asynchronous wait helpers.

hmoco · 2017-04-28T15:08:17Z

@brianjgeiger, can't get this to fail locally, can't get it to pass here. Any ideas?

jamescdavis · 2017-04-28T15:12:28Z

merge develop, then it will fail locally

…r-preprints into feature/tests

jamescdavis · 2017-04-28T16:12:30Z

Also, I wanted to reference ember-cli/ember-cli-qunit#162, which discusses the issue using wait helpers with qunitjs < 2.3.1 and qunitjs/qunit#1148, which was the fix to qunitjs that allows assertions after assert.async(). Switching to ember-cli-qunit 3 was necessary because ember-cli-qunit 2 depends on qunitjs 1.

jamescdavis

LGTM

Tests passing

brianjgeiger

Approved by @jamescdavis

Henrique Moco added 7 commits February 21, 2017 16:15

add facotry-guy, mockjax and acceptance test

8f10f47

Fix module for acceptance, play with content test some more

7728d4c

Flesh out content controller tests

06f1d3f

New component tests:

adbdbff

Adjust author-link tests

7313017

Merge branch 'develop' of http:/CenterForOpenScience/embe…

4f2d582

…r-preprints into feature/tests

Clean up tests and remove unused code

cd2947f

hmoco changed the title ~~[WIP] Feature/tests~~ [EOSF-510] Add example acceptance tests, controller unit tests, and component integration tests Mar 10, 2017

hmoco mentioned this pull request Mar 14, 2017

[EOSF-509] Update factories and have factories be merged to consuming apps CenterForOpenScience/ember-osf#189

Merged

abought reviewed Mar 14, 2017

View reviewed changes

Henrique Moco and others added 2 commits March 14, 2017 15:39

Make sure bower has Faker

227b7ba

Update yarn with pinned ember-osf commit

2091748

hmoco force-pushed the feature/tests branch from 30b5f98 to 2091748 Compare March 20, 2017 20:52

Henrique Moco and others added 6 commits March 20, 2017 21:42

Merge branch 'feature/tests' of http:/CenterForOpenScienc…

ddaa0c9

…e/ember-preprints into feature/tests

Update packages, add stubbing to requests in acceptance tests

ed8659f

Merge branch 'develop' of http:/CenterForOpenScience/embe…

eea027d

…r-preprints into feature/tests

Merge branch 'feature/tests' of http:/CenterForOpenScienc…

0aa4bbd

…e/ember-preprints into feature/tests

Remove obsolete tests

514e5b6

Update module-for-acceptance.js

93b096d

abought previously requested changes Mar 23, 2017

View reviewed changes

Henrique Moco added 5 commits March 27, 2017 09:56

skip do nothing test, start discover page setup

41e49eb

unsure on how to ensure data is available fully prior to hitting andThen

7b2f3b1

Make separates and joins of copyright providers use ', ' rather than ','

781ed81

Play around some more with acceptance/runs

6a20fbd

remove unused tests (acceptance) and clean up/separate other tests

6bfd5c8

hmoco force-pushed the feature/tests branch from 3a2d38e to 6bfd5c8 Compare March 29, 2017 18:19

hmoco changed the title ~~[EOSF-510] Add example acceptance tests, controller unit tests, and component integration tests~~ [EOSF-594] Add example controller unit tests and component integration tests Mar 31, 2017

hmoco added 2 commits April 6, 2017 15:08

Merge branch 'develop' of http:/CenterForOpenScience/embe…

bc75a16

…r-preprints into feature/tests

Merge branch 'feature/tests' of http:/CenterForOpenScienc…

669d71a

…e/ember-preprints into feature/tests

hmoco added 4 commits April 7, 2017 13:30

Attempt to recreate yarn file

480c3d6

Attempt to fix lockfile conflicts

d4e63d3

Merge branch 'feature/tests' of http:/CenterForOpenScienc…

a399056

…e/ember-preprints into feature/tests

Clean up packages, pin ember data facotry guy version

80c272c

jamescdavis suggested changes Apr 25, 2017

View reviewed changes

hmoco added 2 commits April 26, 2017 11:26

Merge branch 'develop' of http:/CenterForOpenScience/embe…

bc0e632

…r-preprints into feature/tests

Add messages to test failures

c12ce72

jamescdavis suggested changes Apr 27, 2017

View reviewed changes

hmoco added 2 commits April 27, 2017 13:45

Make split and joins consistent across copyrightHolders and test for it

a751d6e

Distinguish async behavior on discardbasics, test for copyright holde…

c6bb595

…rs join

jamescdavis suggested changes Apr 28, 2017

View reviewed changes

Revert changes made to submit conroller made in c6bb595 and use wait(…

28208e4

…) test helper instead Upgrading to ember-cli-qunit 3 was required due to bug in v2 that prevents use of asynchronous wait helpers.

Merge branch 'develop' of http:/CenterForOpenScience/embe…

ef7d697

…r-preprints into feature/tests

Move filedownloadUrl test to its component

842875d

hmoco mentioned this pull request Apr 28, 2017

[EOSF-601] Don't call GA event when defaults get loaded, add lodash to preprints #341

Merged

jamescdavis approved these changes Apr 28, 2017

View reviewed changes

brianjgeiger approved these changes Apr 28, 2017

View reviewed changes

brianjgeiger merged commit 65b3d66 into develop Apr 28, 2017

jamescdavis deleted the feature/tests branch November 14, 2018 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hmoco commented Mar 3, 2017 •

edited

Loading

binoculars commented Mar 14, 2017

abought Mar 14, 2017

abought left a comment

abought Mar 23, 2017

abought Mar 23, 2017

abought Mar 23, 2017

abought Mar 23, 2017

abought Mar 23, 2017

abought Mar 23, 2017

abought Mar 23, 2017 •

edited

Loading

abought Mar 23, 2017

abought Mar 23, 2017

abought Mar 23, 2017

jamescdavis left a comment

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

jamescdavis Apr 25, 2017

binoculars commented Apr 25, 2017

jamescdavis commented Apr 25, 2017

jamescdavis left a comment

jamescdavis Apr 27, 2017

jamescdavis left a comment

hmoco commented Apr 28, 2017

jamescdavis commented Apr 28, 2017

jamescdavis commented Apr 28, 2017

jamescdavis left a comment

brianjgeiger left a comment


		moduleForAcceptance('Acceptance \| content');

		test('visiting /content', function(assert) {


		moduleForAcceptance('Acceptance \| discover');

		test('visiting /discover', function(assert) {

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

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

Conversation

hmoco commented Mar 3, 2017 • edited Loading

This PR has been diminished in scope due to problems found with acceptance testing. Ticket previously tracking this PR is https://openscience.atlassian.net/browse/EOSF-510

Ticket

Purpose

Changes

binoculars commented Mar 14, 2017

Choose a reason for hiding this comment

abought left a 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

abought Mar 23, 2017 • 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

jamescdavis left a 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

binoculars commented Apr 25, 2017

jamescdavis commented Apr 25, 2017

jamescdavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamescdavis left a comment

Choose a reason for hiding this comment

hmoco commented Apr 28, 2017

jamescdavis commented Apr 28, 2017

jamescdavis commented Apr 28, 2017

jamescdavis left a comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

hmoco commented Mar 3, 2017 •

edited

Loading

abought Mar 23, 2017 •

edited

Loading