-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
implement spyOnProperty method #5107
Changes from 15 commits
1a3b82a
4cfb737
65d884d
a4a6313
d26337c
7c4844d
afb0e96
05a303d
fc0de22
0a5e083
c0894d9
be83146
e5aa9a6
26b5ddb
95f0ab9
bd44ca5
37093f6
224eb25
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 |
---|---|---|
|
@@ -64,7 +64,11 @@ export default function SpyRegistry(options: Object) { | |
this.respy = allow; | ||
}; | ||
|
||
this.spyOn = function(obj, methodName) { | ||
this.spyOn = function(obj, methodName, accessType?: string) { | ||
if (accessType) { | ||
return this._spyOnProperty(obj, methodName, accessType); | ||
} | ||
|
||
if (obj === void 0) { | ||
throw new Error( | ||
getErrorMsg( | ||
|
@@ -129,6 +133,75 @@ export default function SpyRegistry(options: Object) { | |
return spiedMethod; | ||
}; | ||
|
||
this._spyOnProperty = function(obj, propertyName, accessType = 'get') { | ||
if (!obj) { | ||
throw new Error( | ||
'spyOn could not find an object to spy upon for ' + propertyName + '', | ||
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. There is an empty string at the end. Can we end the sentence with a 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 will do it. 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. |
||
); | ||
} | ||
|
||
if (!propertyName) { | ||
throw new Error('No property name supplied'); | ||
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. Same as above 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 will fix it. |
||
} | ||
|
||
let descriptor; | ||
try { | ||
descriptor = Object.getOwnPropertyDescriptor(obj, propertyName); | ||
} catch (e) { | ||
// IE 8 doesn't support `definePropery` on non-DOM nodes | ||
} | ||
|
||
if (!descriptor) { | ||
throw new Error(propertyName + ' property does not exist'); | ||
} | ||
|
||
if (!descriptor.configurable) { | ||
throw new Error(propertyName + ' is not declared configurable'); | ||
} | ||
|
||
if (!descriptor[accessType]) { | ||
throw new Error( | ||
'Property ' + propertyName + ' does not have access type ' + accessType, | ||
); | ||
} | ||
|
||
if (obj[propertyName] && isSpy(obj[propertyName])) { | ||
if (this.respy) { | ||
return obj[propertyName]; | ||
} else { | ||
throw new Error( | ||
getErrorMsg(propertyName + ' has already been spied upon'), | ||
); | ||
} | ||
} | ||
|
||
const originalDescriptor = descriptor; | ||
const spiedProperty = createSpy(propertyName, descriptor[accessType]); | ||
let restoreStrategy; | ||
|
||
if (Object.prototype.hasOwnProperty.call(obj, propertyName)) { | ||
restoreStrategy = function() { | ||
Object.defineProperty(obj, propertyName, originalDescriptor); | ||
}; | ||
} else { | ||
restoreStrategy = function() { | ||
delete obj[propertyName]; | ||
}; | ||
} | ||
|
||
currentSpies().push({ | ||
restoreObjectToOriginalState: restoreStrategy, | ||
}); | ||
|
||
const spiedDescriptor = Object.assign({}, descriptor, { | ||
[accessType]: spiedProperty, | ||
}); | ||
|
||
Object.defineProperty(obj, propertyName, spiedDescriptor); | ||
|
||
return spiedProperty; | ||
}; | ||
|
||
this.clearSpies = function() { | ||
const spies = currentSpies(); | ||
for (let i = spies.length - 1; i >= 0; i--) { | ||
|
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.
why was this changed? This seems unrelated.
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.
i got runtime errors when developing this feature when
uncheckedCount
is falsy. do you think that this change is not required?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.
I think this should be changed in a separate commit, with a test that is fixed by your change. Please revert it from this PR and send one for this issues specifically. Thank you.