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

Fix api options errors #29

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix api options errors #29

wants to merge 5 commits into from

Conversation

henrahmagix
Copy link
Contributor

Fix #28

@incuna/js Please merge, ta!

To speed up e2e tests and make them easier to read, since lots of files
changing triggers lots of runs of the suites, and sometimes there's an
error because a file is being written to when the tests run.

(function () {

var helpers = window._helpers = window._helpers || {};
Copy link

Choose a reason for hiding this comment

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

window._helpers twice?

Copy link
Contributor Author

@henrahmagix henrahmagix Jun 2, 2017

Choose a reason for hiding this comment

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

Instantiates window._helpers if it's not already setup, whilst also returning the same object to helpers so it can be altered and automatically "exported" as window._helpers.

Perhaps this would read better?

var helpers = window._helpers = (window._helpers || {});

Copy link

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7e4a0a3

var factoryMethods = options.factory.methods;


describe(moduleName + ': getting fields from OPTIONS request', function () {
Copy link
Member

Choose a reason for hiding this comment

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

We have previously found (in Python tests) that too much automation in the tests results in issues debugging the tests and understanding them. The result has been that we try to make tests as simple as possible even if involves them not being DRY.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this replication it seems that there should be a single service that we could test with a single test that all directives use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's exactly what I was about to type: that this DRY test shows how the source can be made DRY to allow for simpler tests =D

Copy link
Member

Choose a reason for hiding this comment

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

In other words, rather than writing DRY tests it would be better to DRY the code to a service that does something like:

(response) =>  response.data.actions && (response.data.actions.POST || response.data.actions.PUT)

Copy link
Member

Choose a reason for hiding this comment

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

The test the service / factory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that it would be a better task to think about what more can be service-ised, what similar concepts appear in multiple modules and thus have duplicated code.

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.

3 participants