-
-
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
Fix mocking inherited static properties and prototype-less objects #7003
Conversation
50af289
to
cd737c6
Compare
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.
This is great!
|
||
expect(typeof ClassBarMock.foo).toBe('function'); | ||
expect(typeof ClassBarMock.fooProp).toBe('function'); | ||
expect(ClassBarMock.foo.mock).not.toBeUndefined(); |
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.
we have toBeDefined
expect(BarMock.bind).toBe(mockGlobals.Function.prototype.bind); | ||
}); | ||
|
||
it('does not mock methods from Function.prototype (in mock context)', () => { |
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.
This is why I had to revert #6921. In this specific test we use builtins from the host, not the ones passed to ModuleMockerClass
. This test covers that too.
cd737c6
to
19dfec9
Compare
19dfec9
to
24cd5c4
Compare
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.
Address feedback before merging.
class Foo {} | ||
class Bar extends Foo {} | ||
Bar; | ||
`, |
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.
There's a weird alignment going on here... Do instead:
const Bar = vm.runInContext(
`
class Foo {}
class Bar extends Foo {}
Bar;
`,
mockContext,
);
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.
That's all prettier 🤷🏻♂️
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.
you can add a space inside the literal to align the backticks, prettier won't strip them
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.
Correct; once you're in backtick mode, prettier is not changing the contents.
expect(BarMock.bind).toBe(mockGlobals.Function.prototype.bind); | ||
}); | ||
|
||
it('does not mock methods from RegExp.prototype', () => { |
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.
Didn't we say we were going to remove the RegExp
whitelisting? (i.e. either all or none).
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.
That would be a breaking change. RegExp
is still a special case because we don't mock their methods (and the rest of built-ins are already managed in collections or here).
` | ||
const bar = /bar/; | ||
bar; | ||
`, |
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.
Weird alignment again...
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes #6994 and #6995, and generalizes some code from jest-mock.
Re-applies #6921 fixing its bugs.
Test plan
Added new test cases and tested in FB infra.