Skip to content

Commit

Permalink
Add tests for RecordArray.destroy
Browse files Browse the repository at this point in the history
* leave note for future work, e.g. the cleanup that is happening should likely not happen.
  • Loading branch information
stefanpenner committed Oct 19, 2016
1 parent c0a4b7c commit cfe32d3
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 5 deletions.
13 changes: 10 additions & 3 deletions addon/-private/system/record-arrays/record-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ export default Ember.ArrayProxy.extend(Ember.Evented, {
},

_dissociateFromOwnRecords() {
this.get('content').forEach(record => {
let recordArrays = record._recordArrays;
this.get('content').forEach(internalModel => {
let recordArrays = internalModel._recordArrays;

if (recordArrays) {
recordArrays.delete(this);
Expand All @@ -218,7 +218,14 @@ export default Ember.ArrayProxy.extend(Ember.Evented, {
willDestroy() {
this._unregisterFromManager();
this._dissociateFromOwnRecords();
set(this, 'content', undefined);
// TODO: we should not do work during destroy:
// * when objects are destroyed, they should simply be left to do
// * if logic errors do to this, that logic needs to be more careful during
// teardown (ember provides isDestroying/isDestroyed) for this reason
// * the exception being: if an dominator has a reference to this object,
// and must be informed to release e.g. e.g. removing itself from th
// recordArrayMananger
set(this, 'content', null);
set(this, 'length', 0);
this._super(...arguments);
},
Expand Down
50 changes: 48 additions & 2 deletions tests/unit/record-arrays/record-array-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import DS from 'ember-data';
import Ember from 'ember';
import { module, test } from 'qunit';

const { get, RSVP } = Ember;
const { get, RSVP, run } = Ember;
const { RecordArray } = DS;

module('unit/record-arrays/record-array - DS.RecordArray');
Expand Down Expand Up @@ -178,6 +178,7 @@ test('#removeInternalModel', function(assert) {

function internalModelFor(record) {
return {
_recordArrays: undefined,
getRecord() { return record; }
};
}
Expand Down Expand Up @@ -207,5 +208,50 @@ test('#save', function(assert) {

return result.then(result => {
assert.equal(result, result, 'save promise should fulfill with the original recordArray');
})
});
});

test('#destroy', function(assert) {
let didUnregisterRecordArray = 0;
let didDissociatieFromOwnRecords = 0;
let model1 = { };
let internalModel1 = internalModelFor(model1);

// TODO: this will be removed once we fix ownership related memory leaks.
internalModel1._recordArrays = {
delete(array) {
didDissociatieFromOwnRecords++;
assert.equal(array, recordArray);
}
};
// end TODO:

let recordArray = RecordArray.create({
content: Ember.A([internalModel1]),
manager: {
unregisterRecordArray(_recordArray) {
didUnregisterRecordArray++;
assert.equal(recordArray, _recordArray);
}
}
});

assert.equal(get(recordArray, 'isDestroyed'), false, 'should not be destroyed');
assert.equal(get(recordArray, 'isDestroying'), false, 'should not be destroying');

run(() => {
assert.equal(get(recordArray, 'length'), 1, 'before destroy, length should be 1');
assert.equal(didUnregisterRecordArray, 0, 'before destroy, we should not yet have unregisterd the record array');
assert.equal(didDissociatieFromOwnRecords, 0, 'before destroy, we should not yet have dissociated from own record array');
recordArray.destroy();
});

assert.equal(didUnregisterRecordArray, 1, 'after destroy we should have unregistered the record array');
assert.equal(didDissociatieFromOwnRecords, 1, 'after destroy, we should have dissociated from own record array');
recordArray.destroy();

assert.equal(get(recordArray, 'content'), null);
assert.equal(get(recordArray, 'length'), 0, 'after destroy we should have no length');
assert.equal(get(recordArray, 'isDestroyed'), true, 'should be destroyed');
});

0 comments on commit cfe32d3

Please sign in to comment.