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

chore(test): clean integration test suite #164

Closed

Conversation

jniles
Copy link
Collaborator

@jniles jniles commented Mar 4, 2016

This commit cleans up the application's integration test suite in
several ways:

  1. Ensure all tests using UUIDs are using node-uuid, and ensuring
    they are using it properly (uuid.v4() over uuid()).
  2. Removes extraneous comments.
  3. Transitions many modules to using test helpers exclusively. Removes
    the errorKeys property from the test helpers -- all errors should be
    caught via helpers.api.errored().
  4. [perf] Improves testing logs and performance by using before() instead of beforeEach() to log the agent in.

The goal is to have cleaner, more maintainable tests for developers to
use as references when writing future integration tests.

@jniles jniles force-pushed the patches-integration-testing branch from 605e437 to 512b21d Compare March 5, 2016 08:08
This commit cleans up the application's integration test suite in
several ways:
 1. Ensure all tests using UUIDs are using `node-uuid`, and ensuring
 they are using it properly (`uuid.v4()` over `uuid()`).
 2. Removes extraneous comments.
 3. Transitions many modules to using test helpers exclusively.  Removes
 the `errorKeys` property from the test helpers -- all errors should be
 caught via `helpers.api.errored()`.

The goal is to have cleaner, more maintainable tests for developers to
use as references when writing future integration tests.
To improve performance, we are logging the agent used in the helpers in
once before the test suite instead of before each request.  This reduces
the overall number of requests, allows cleaner logs, and reduces the
overall testing time.
@jniles jniles force-pushed the patches-integration-testing branch from 512b21d to 060cdce Compare March 5, 2016 08:13
@DedrickEnc
Copy link
Contributor

I am happy to notice that, you deleted used test files

The integration test was difficult at the beginning because every one was supposed to learn the API, but now every one is so quick, and the same situation is observable for the end to end test, it is so difficult but every one will be quick with the time.

You introduces new interfaces to the helpers.js file.

So I want to know, since this change will affect the productivity of every one :

  1. What is the motivation which leads you to almost rewrite the integration test?
  2. Did you discuss that with the team? (did you at least create an issue?)
  3. In some situation testing an API can necessitate some particulars checks which are very tailed to the business logic, do you think these generics methods (listed, errored ...) are enough to satisfy every developers during the test?

e.g : user.js
without helpers.api

expect(res).to.have.status(200);
expect(res.body).to.not.be.empty;
expect(res.body).to.have.length(1);     
expect(res.body[0]).to.have.keys('id', 'unit_id');
expect(res.body[0].unit_id).to.equal(0);

with helpers api

helpers.api.listed(res, 1);
expect(res.body[0]).to.have.keys('id', 'unit_id');
expect(res.body[0].unit_id).to.equal(0);

4.. Why some files are not entirely covered by the change, e.g employee.js

@jniles
Copy link
Collaborator Author

jniles commented Mar 5, 2016

Motivation
Everyone implemented a slightly different method of testing. Some developers checked for json, others did not. Some developers checkout .to.have.length(N), others simply check .not.empty. There was was a lot of boilerplate code.

The helper methods were developed to allow our team to focus on testing situations, rather than simply writing lots of code. Since most APIs behave the same (all return JSON encoding, create statements should send back either an ID or a UUID, etc...), it makes sense to move these checks into a place they can be used easily and frequently.

No, I did not discuss it with the team. That discussion is free to happen here. There are many times when I've created issues that are never read by @IMA-WorldHealth/local-contributors , so I am experimenting with new ways to show my ideas.

Of course generic methods will not be enough to replace specific cases. Our development team should still be thinking about, and writing specific tests. This just helps write common tests faster.

Some files are not entirely covered because I didn't get around to completing them.

It is evident that these types of changes are not appreciated. So, I am closing this pull request.

@jniles jniles closed this Mar 5, 2016
@jniles jniles deleted the patches-integration-testing branch March 5, 2016 19:23
sfount pushed a commit that referenced this pull request Jan 19, 2017
This commit refactors the complex vouchers convention payment tool to
import all debtor groups -- since the client has no way of setting the
`is_convention` flag.

A minor bug was fixed where the form was not set to `$pristine` after
completing the payment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants