thefrontside / emberx-form

Form/field components for working with changesets
MIT License
3 stars 1 forks source link

Migrate to Mocha -> Add Integration Tests #3

Closed flexyford closed 8 years ago

flexyford commented 8 years ago

Ready for Review

All tests have been migrated to ember-mocha.

In tests/integration/component/x-form.js we started writing first set of tests which mock pending, resolved, and rejected promises.

The next set of tests should mirror the x-form tests in honeywell

flexyford commented 8 years ago

@cowboyd I'm curious how to setup the a promise in the pending state.

And then in the next (nested) test, we can call resolve("Success") and reject("Error")? I tried to use honeywell's promise mock as an example, but did not find where we explicitly reject or resolve the promise.

I came up with this instead which sets the submit action to a different type of promise for each test.

describe('when a submission is in flight', function() {
  beforeEach(() => {
    this.set('submit', () => new Ember.RSVP.Promise(() => {}) );
  });
});
describe('when a submission is resolved', function() {
  beforeEach(() => {
    this.set('submit', () => Ember.RSVP.Promise.resolve('success'));
  });
});
describe('when a submission is rejected', function() {
  beforeEach(() => {
    this.set('submit', () => Ember.RSVP.Promise.reject('error'));
  });
});
cowboyd commented 8 years ago

@flexyford

I think there are two things, first submit needs to be a function that returns a promise.

Second, you should be able to use Ember.RSVP.defer().promise instead of the promise mock.

flexyford commented 8 years ago

Ah, breaker-social-promo.js in Conde Nast Scene-Two-Frontend had a defer. I'll check that out.

flexyford commented 8 years ago

@cafreeman I believe I've setup the first major test, resolving and rejecting submission. Give it a review, and I hope it's ready for merge.

Robdel12 commented 8 years ago

@flexyford @cafreeman @cowboyd maybe we take this repo public today? I'd like to get CI running and can't do that with a private repo

cowboyd commented 8 years ago

@Robdel12 Before we go public, there are a couple of public API issues it will make our life eaiser to settle now rather than later:

First is naming conventions for component attributes. kebab case vs camel case.

Another is that it seems like the action should be form.actions.submit vs form.actions.onSubmit onX implies that this is something that gets called after the action fires, but the trigger of the action should be named after the action itself. Or, if we want to have a prefix for symmetry it should be something like form.actions.doSubmit, but the fact that it's already nested under the form.actions namespace seems to make that obvious.

Robdel12 commented 8 years ago

@cowboyd I'm talking about making the repo public, not really publishing :P

I don't think anyone will stumble across the repo in the mean time

flexyford commented 8 years ago

I think all the issues @cowboyd laid out should be resolved before going into master at AltSchool

Robdel12 commented 8 years ago

@flexyford that's too late :p

cafreeman commented 8 years ago

@cowboyd I've pushed an update that chances onSubmit to submit, and does the same for cancel. Definitely agree with you there.

I think onSuccess and onError still make sense, given that they are actually reacting to something else in the sequence of events.

cafreeman commented 8 years ago

With regard to the naming convention, I think everything is currently camelCase. I'm a fan of using camelCase for things aren't actually HTML attributes (so in this case, all the props are camelCase because they're all arguments to the component constructor).

What do you think?

cafreeman commented 8 years ago

Other than the comments above, this LGTM. @Robdel12 @flexyford

cowboyd commented 8 years ago

I think that the naming convention is greater than just this library. It's more of an eco-system convention.

I agree that JavaScript values should be camel cased, and certainly the right side of the binding is:

<my-component on-event=doSomethingQuickly/>

But the lhs is never a JS value. It's just an attribute name that indexes a JS value.

My understanding might need double checking as to the eco-system conventions. Maybe we can ask the tweet-o-sphere?

cafreeman commented 8 years ago

In the case where things are actually going to end up in the DOM, I totally agree with you. However, in the case of actions, doing something like {{x-form onSuccess=(action 'doSomething')}} will just result in <form>...</form> in the DOM, since the action "attributes" are actually just a shortcut to function arguments.

Essentially, what we're doing is something like new XForm(onSuccess=doSomething), I believe.

cafreeman commented 8 years ago

For reference, I think the guides lean in the "camelCase component properties" direction in examples like this:

https://guides.emberjs.com/v2.7.0/components/wrapping-content-in-a-component/#toc_sharing-component-data-with-its-wrapped-content

cafreeman commented 8 years ago

I'm going to go ahead and merge this so we can get this public and on CI, but i've moved the naming convention to an issue so we can continue it there: https://github.com/thefrontside/emberx-form/issues/4