Skip to content
This repository has been archived by the owner on Apr 18, 2020. It is now read-only.

Add tests #53

Merged
merged 1 commit into from
Mar 2, 2015
Merged

Add tests #53

merged 1 commit into from
Mar 2, 2015

Conversation

jrschumacher
Copy link
Contributor

Added some basic tests for example. In progress

Todo:

Todo model

  • PUT test - need to test updating a todo
  • remote method stats test
  • filter tests

User model

  • CRUD tests - the model file looks the same as todo

@bajtos
Copy link
Member

bajtos commented Jan 14, 2015

@jrschumacher thank you for the pull request. Please move the test file to server/test.

"test": "grunt test",
"build": "bower install; grunt build"
"build": "bower install; grunt build",
"test": "./node_modules/loopback-testing/node_modules/.bin/mocha common/models/test"
Copy link
Member

Choose a reason for hiding this comment

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

npm test should run both server and client tests. Please keep grunt test and rework the Gruntfile: rename the task test to test:client, add a test:server task to run Mocha tests in server/test, and add a test task to run test:server and test:client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos is there a specific way to get grunt to run a command line or should I look at adding grunt-mocha or grunt-exec?

@bajtos bajtos changed the title Add tests [WIP] Add tests Jan 14, 2015
@jrschumacher
Copy link
Contributor Author

Sure will do.

@jrschumacher
Copy link
Contributor Author

@bajtos fixed the grunt issue

thank you for the pull request. Please move the test file to server/test.

How should I be formatted? I left it in the common models because I found your stubs there https://github.com/strongloop/loopback-example-full-stack/tree/master/common/models/test

@bajtos
Copy link
Member

bajtos commented Jan 15, 2015

I left it in the common models because I found your stubs there https://github.com/strongloop/loopback-example-full-stack/tree/master/common/models/test

Well, those stubs would be for pure unit-tests not using REST transport.

How should I be formatted?

Just move the file, I think the current content is ok for the first version. We can always improve it later.

quiet: false, // Optionally suppress output to standard out (defaults to false)
clearRequireCache: false // Optionally clear the require cache before running tests (defaults to false)
},
src: ['common/models/test/**/*.js']
Copy link
Member

Choose a reason for hiding this comment

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

This should be server/test/*.js, the usual convention is to pick up only top-level JS files.

Tests in common folder would be run by grunt test:common, but don't worry about that now.

@jrschumacher
Copy link
Contributor Author

@bajtos I just noticed that common/model/user.json is essentially the same as common/model/todo.json. Is this a mistake?

I am omitting the user tests since it looks the same

@jrschumacher
Copy link
Contributor Author

@bajtos I am done. Not going to do the filter test though. Not enough time.

I can rebase my work as needed.

// Get todo stats
lt.describe.whenCalledRemotely('POST', '/api/Todos/stats', function() {
it('should respond with total, remaining and 1 completed', function() {
lt.it.shouldBeAllowed();
Copy link
Member

Choose a reason for hiding this comment

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

You cannot nest lt.it inside it blocks. Move it outside.

@bajtos
Copy link
Member

bajtos commented Jan 16, 2015

Not going to do the filter test though. Not enough time.

That's fine. It's better to have some tests than no tests at all.

I am done.

Cool. I reviewed the code, see the comment above.

@ritch Is the test code in this patch using your test helpers correctly? I find nested describe.whenCalledRemotely rather unusual, also the spec output of the test suite does not really resemble a specification. Should this use plain supertest instead?

@jrschumacher
Copy link
Contributor Author

@bajtos I'd be happy to unnest. When I was creating it I assumed the database was cleared after every request as well as the async nature of whenCalledRemotely which is why I chose to nest. If it makes sense not to nest I'd be happy to, but I will need some help in determining how to avoid the potential race condition

@jrschumacher
Copy link
Contributor Author

@bajtos here is the proof to the issue with beforeEach loopback-testing/97c64458

@bajtos
Copy link
Member

bajtos commented Jan 20, 2015

I believe the test should call beforeEach.givenModel at the beginning. It will install an after handler to remove all existing model instances after each tests.

testApp.use(loopback.rest());
helpers.beforeEach.withApp(testApp);
helpers.beforeEach.givenModel(testModel);

Note that the helper also creates a single new model instances and stores it on the test context object. I guess this can be improved by splitting the method givenModel into two helpers:

  • usingModel that checks that the model is configured in the app and installs the afterEach hook to delete all instances
  • givenModel that creates the model instance and calls usingModel under the hood

Alternatively, you can simply ignore these extra helpers and manually write afterEach hook that deletes all model instances.

@ritch thoughts?

@bajtos
Copy link
Member

bajtos commented Jan 27, 2015

@jrschumacher This is waiting until strongloop-archive/loopback-testing#40 is landed & released, is that correct?

@jrschumacher
Copy link
Contributor Author

@bajtos I would suggest that. When it does I'll clear the database before the count so the count makes more sense.

@bajtos bajtos added the waiting label Feb 2, 2015
@bajtos
Copy link
Member

bajtos commented Feb 23, 2015

strongloop-archive/loopback-testing#40 has been landed and released as [email protected].

@jrschumacher could you please finish this pull request into the state where it can be landed?

@bajtos bajtos removed the waiting label Feb 23, 2015
@jrschumacher
Copy link
Contributor Author

@bajtos I believe its good to go please test

@ritch
Copy link
Member

ritch commented Mar 2, 2015

@bajtos ping

@bajtos bajtos changed the title [WIP] Add tests Add tests Mar 2, 2015
bajtos added a commit that referenced this pull request Mar 2, 2015
@bajtos bajtos merged commit e019d82 into strongloop:master Mar 2, 2015
@bajtos
Copy link
Member

bajtos commented Mar 2, 2015

Landed, thank you for the contribution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants