Fix and tests for unloadRecord => findRecord issue #5011
Conversation
| store.unloadRecord(record); | ||
| assert.equal(record.isDestroying, true, 'the record is destroying'); | ||
| assert.equal(internalModel.currentState.stateName, 'root.empty', 'We are unloaded after unloadRecord'); | ||
| store.findRecord('person', '1'); |
There was a problem hiding this comment.
this should likely cancel the above unloadRecords's _checkForOrphanedInternalModels and unset _isDematerializing, so it can be back in a pristine place, ready for a findRecord
runspired
force-pushed
the
fix-unload-find-bug
branch
2 times, most recently
from
8059897
to
cd5e254
Compare
runspired
changed the title
| assert.notEqual(timer, null, 'We scheduled destroy'); | ||
| store.unloadRecord(record); | ||
| let newTimer = internalModel._scheduledDestroy; | ||
| assert.equal(newTimer, timer, 'We scheduled destroy only once'); |
There was a problem hiding this comment.
is there a way for us to test the observable behavior of this rather then the implementation? Specifically, i suspect this will cause issues \w resurrection, and it should be possible to test that directly.
|
|
||
| run(function() { | ||
| store.unloadRecord(record); | ||
| let timer = internalModel._scheduledDestroy; |
There was a problem hiding this comment.
we should aim to test observable behavior. This seems to tied to implementation details.
There was a problem hiding this comment.
How about two calls to scheduleDestroy followed by a cancelDestroy and testing that isDestroyed is false?
There was a problem hiding this comment.
yup, something like that sounds good. I just want to maximize test value.
| if (this._isDematerializing === true) { | ||
| this._isDematerializing = false; | ||
| run.cancel(this._scheduledDestroy); | ||
| this._scheduledDestroy = null; |
There was a problem hiding this comment.
Any reason to not put this outside the above if statement? I suspect always nulling this one here, will prevent us from accidentally not nulling it out sometimes.
There was a problem hiding this comment.
Good catch. The intent was to avoid unnecessary work if we weren't in a state that needed to be cleaned up, but this isn't the correct check for that. Moved to just always resetting all three things.
…dRecord, adds test coverage
runspired
force-pushed
the
fix-unload-find-bug
branch
from
63371a9
to
d0b64ee
Compare
|
@bmac we likely need to bugfix release this guy. |
The underlying issue was that the internalModel was not cleaning up it's
destroycall and flags when being rematerialized immediately.