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

make createRecord sync and remove unnecessary run usage in tests #5415

Merged
merged 1 commit into from Apr 5, 2018

Conversation

runspired
Copy link
Contributor

refactor all the test usage or run-wrapping for createRecord, we really should run prettier

resolves #5247 because I <3 @rwjblue

@sly7-7
Copy link
Contributor

sly7-7 commented Apr 4, 2018

Any idea about the failing test against ember 2-12 ?

@runspired
Copy link
Contributor Author

@sly7-7 I'm not sure why ember 2.12 specifically does not pass; however, I dug into the test and realized that it was a bogus test. After rewriting it to test what it claims to be writing I find that this feature has been broken for a very very long time.


test('it is possible to filter created records by isReloading', function(assert) {
  customAdapter(env, DS.Adapter.extend({
    findRecord(store, type, id, snapshot) {
      return {
        data: {
          id: 1,
          type: 'person',
          attributes: {
            name: 'Tom Dalle'
          }
        }
      };
    }
  }));

  let filter = store.filter('person', person => {
    return !person.get('isReloading');
  });

  let person = store.createRecord('person', {
    id: '1',
    name: 'Tom Dale'
  });

  assert.equal(person.get('isReloading'), false, 'The person is not reloading');
  assert.equal(filter.get('length'), 1, 'the filter correctly returned a available record');

  run(() => {
    person.reload();
    assert.equal(person.get('isReloading'), true, 'The person is reloading');
    assert.equal(filter.get('length'), 0, 'the filter correctly removes an isReloading record');
  });

  assert.equal(person.get('isReloading'), false, 'The person is done reloading');
  assert.equal(filter.get('length'), 1, 'the filter correctly returned a reloaded record');
});

The assertion for length 0 will not trigger even though the state at that point is confirmed to be isReloading. We don't really support filters anymore, especially in this situation where we would have to be overly noisy (bad for perf) for all record arrays for an edge case. Someone desiring this behavior would be much better suited to filter via @each.isReloading. I'm removing this test.

@runspired
Copy link
Contributor Author

seems I have some async test leaks to find

…ly sohuld run prettier

minor style tweak

remove invalid filter test
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.

store.createRecord should be async?
4 participants