-
Notifications
You must be signed in to change notification settings - Fork 86
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: storeName can be used in createInstance and instance functions. #119
base: master
Are you sure you want to change the base?
Changes from 5 commits
82856dc
0503e19
4576554
7995159
4421f21
706dd61
197d7e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,6 +150,18 @@ describe('Module: LocalForageModule', function() { | |
}, done); | ||
}); | ||
|
||
it('should have an iterationNumber with a 1-index', function(done) { | ||
var count; | ||
|
||
$localForage.iterate(function(value, key, iterationNumber) { | ||
count = iterationNumber; | ||
}).then(function() { | ||
stopDigests(interval); | ||
expect(count).toEqual(3); | ||
done(); | ||
}, done); | ||
}) | ||
|
||
it('key/value filter should work', function(done) { | ||
//test key filter | ||
$localForage.iterate(function(value, key) { | ||
|
@@ -465,4 +477,94 @@ describe('Module: LocalForageModule', function() { | |
}, done); | ||
}); | ||
}); | ||
|
||
describe("createInstance", function () { | ||
beforeEach(function () { | ||
$localForage.createInstance({ | ||
name: 'DUPLICATE_INSTANCE_NAME' | ||
}); | ||
}); | ||
it('should create a new instance', function () { | ||
expect($localForage.createInstance({ | ||
name: 'TEST_INSTANCE' | ||
})).toBeDefined(); | ||
}); | ||
|
||
it('should throw error if trying to create duplicate instance.', | ||
function () { | ||
expect($localForage.createInstance.bind($localForage, { | ||
name: 'DUPLICATE_INSTANCE_NAME' | ||
})).toThrowError(/already defined/); | ||
}); | ||
|
||
it('should create instance with same name, different storeName', | ||
function () { | ||
expect($localForage.createInstance.bind($localForage, { | ||
name: 'DUPLICATE_INSTANCE_NAME', | ||
storeName: 'DIFFERENT_STORE_NAME' | ||
})).not.toThrowError(/already defined/); | ||
}); | ||
}); | ||
|
||
describe("instance", function () { | ||
var $q, interval; | ||
beforeEach(function () { | ||
$localForage.createInstance({ | ||
name: 'TEST_INSTANCE_1' | ||
}); | ||
$localForage.createInstance({ | ||
name: 'TEST_INSTANCE_2', | ||
storeName: 'TEST_STORE_NAME_1' | ||
}); | ||
$localForage.createInstance({ | ||
name: 'TEST_INSTANCE_2', | ||
storeName: 'TEST_STORE_NAME_2' | ||
}); | ||
inject(function (_$q_) { | ||
$q = _$q_; | ||
}); | ||
interval = triggerDigests(); | ||
}); | ||
|
||
afterEach(function () { | ||
stopDigests(interval); | ||
}); | ||
|
||
it('should get instance by name', function () { | ||
expect($localForage.instance({ | ||
name: 'TEST_INSTANCE_1' | ||
})).toBeDefined(); | ||
}); | ||
|
||
it('should throw exception if instance not exists', function () { | ||
expect($localForage.instance.bind($localForage, { | ||
name: 'NON_EXISTENT' | ||
})).toThrowError(); | ||
}); | ||
|
||
it('should get instances with same name, different storeNames', | ||
function (done) { | ||
var instance1 = $localForage.instance({ | ||
name: 'TEST_INSTANCE_2', | ||
storeName: 'TEST_STORE_NAME_1' | ||
}); | ||
var instance2 = $localForage.instance({ | ||
name: 'TEST_INSTANCE_2', | ||
storeName: 'TEST_STORE_NAME_2' | ||
}); | ||
$q.all([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to exercise expect(instance1).not.toBe(instance2) will suffice to show that they are two separate references since they are reference types, which will clean up the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, they are the same LocalForageInstance references. but the createInstance behavior in localforage library drove me to write such tests because; But you're right, in that case that is not a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. From my reading of the code you submitted, the second I would say as long as: var fooBar = { name: 'foo', storeName: 'bar' };
expect($localForage.instance(fooBar)).toBe($localForage.instance(fooBar)); is true, let's just simplify the test case to make it a little easier to understand what Might even be worth adding that as a test case, just to show that we're passing the instance by reference and expect that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have already applied your suggestion and changed the test to not.toBe(instance). my example was not for the $localForage, but for localforage itself. in the localforage library, there is no
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I followed that example you posted. Thanks for the clarification! |
||
instance1.setItem('key', 'val1'), | ||
instance2.setItem('key', 'val2') | ||
]).then(function () { | ||
return $q.all([ | ||
instance1.getItem('key').then(function (val) { | ||
expect(val).toEqual('val1'); | ||
}), | ||
instance2.getItem('key').then(function (val) { | ||
expect(val).toEqual('val2'); | ||
}) | ||
]); | ||
}).then(done); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this added code is indented 4 spaces, but the rest of the code base is 2-space indent. Please conform, to keep a consistent style.