Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 20, 2018
Merged

Fix mocking inherited static properties and prototype-less objects #7003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
- `[jest-haste-map]` Do not visit again files with the same sha-1 ([#6990](https://github.com/facebook/jest/pull/6990))
- `[jest-jasmine2]` Fix memory leak in Error objects hold by the framework ([#6965](https://github.com/facebook/jest/pull/6965))
- `[jest-haste-map]` Fixed Haste whitelist generation for scoped modules on Windows ([#6980](https://github.com/facebook/jest/pull/6980))
- `[jest-mock]` Fix inheritance of static properties and methods in mocks ([#7003](https://github.com/facebook/jest/pull/7003))
- `[jest-mock]` Fix mocking objects without `Object.prototype` in their prototype chain ([#7003](https://github.com/facebook/jest/pull/7003))

### Chore & Maintenance

Expand Down
144 changes: 139 additions & 5 deletions packages/jest-mock/src/__tests__/jest_mock.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ const vm = require('vm');

describe('moduleMocker', () => {
let moduleMocker;
let mockContext;
let mockGlobals;

beforeEach(() => {
const mock = require('../');
const global = vm.runInNewContext('this');
moduleMocker = new mock.ModuleMocker(global);
mockContext = vm.createContext();
mockGlobals = vm.runInNewContext('this', mockContext);
moduleMocker = new mock.ModuleMocker(mockGlobals);
});

describe('getMetadata', () => {
Expand Down Expand Up @@ -137,7 +140,7 @@ describe('moduleMocker', () => {

expect(typeof foo.nonEnumMethod).toBe('function');

expect(mock.nonEnumMethod.mock).not.toBeUndefined();
expect(mock.nonEnumMethod.mock).toBeDefined();
expect(mock.nonEnumGetter).toBeUndefined();
});

Expand Down Expand Up @@ -180,9 +183,140 @@ describe('moduleMocker', () => {

expect(typeof foo.foo).toBe('function');
expect(typeof instanceFooMock.foo).toBe('function');
expect(instanceFooMock.foo.mock).not.toBeUndefined();
expect(instanceFooMock.foo.mock).toBeDefined();

expect(instanceFooMock.toString.mock).not.toBeUndefined();
expect(instanceFooMock.toString.mock).toBeDefined();
});

it('mocks ES2015 non-enumerable static properties and methods', () => {
class ClassFoo {
static foo() {}
}
ClassFoo.fooProp = () => {};

class ClassBar extends ClassFoo {}

const ClassBarMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(ClassBar),
);

expect(typeof ClassBarMock.foo).toBe('function');
expect(typeof ClassBarMock.fooProp).toBe('function');
expect(ClassBarMock.foo.mock).toBeDefined();
expect(ClassBarMock.fooProp.mock).toBeDefined();
});

it('mocks methods in all the prototype chain (null prototype)', () => {
const Foo = Object.assign(Object.create(null), {foo() {}});
const Bar = Object.assign(Object.create(Foo), {bar() {}});

const BarMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(Bar),
);
expect(typeof BarMock.foo).toBe('function');
expect(typeof BarMock.bar).toBe('function');
});

it('does not mock methods from Object.prototype', () => {
const Foo = {foo() {}};
const Bar = Object.assign(Object.create(Foo), {bar() {}});

const BarMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(Bar),
);

expect(BarMock).toBeInstanceOf(mockGlobals.Object);
expect(
Object.prototype.hasOwnProperty.call(BarMock, 'hasOwnProperty'),
).toBe(false);
expect(BarMock.hasOwnProperty).toBe(
mockGlobals.Object.prototype.hasOwnProperty,
);
});

it('does not mock methods from Object.prototype (in mock context)', () => {
const Bar = vm.runInContext(
`
const Foo = { foo() {} };
const Bar = Object.assign(Object.create(Foo), { bar() {} });
Bar;
`,
mockContext,
);

const BarMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(Bar),
);

expect(BarMock).toBeInstanceOf(mockGlobals.Object);
expect(
Object.prototype.hasOwnProperty.call(BarMock, 'hasOwnProperty'),
).toBe(false);
expect(BarMock.hasOwnProperty).toBe(
mockGlobals.Object.prototype.hasOwnProperty,
);
});

it('does not mock methods from Function.prototype', () => {
class Foo {}
class Bar extends Foo {}

const BarMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(Bar),
);

expect(BarMock).toBeInstanceOf(mockGlobals.Function);
expect(Object.prototype.hasOwnProperty.call(BarMock, 'bind')).toBe(false);
expect(BarMock.bind).toBe(mockGlobals.Function.prototype.bind);
});

it('does not mock methods from Function.prototype (in mock context)', () => {
Copy link
Contributor Author

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.

const Bar = vm.runInContext(
`
class Foo {}
class Bar extends Foo {}
Bar;
`,
Copy link
Contributor

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,
      );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all prettier 🤷🏻‍♂️

Copy link
Member

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

Copy link
Contributor

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.

mockContext,
);

const BarMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(Bar),
);

expect(BarMock).toBeInstanceOf(mockGlobals.Function);
expect(Object.prototype.hasOwnProperty.call(BarMock, 'bind')).toBe(false);
expect(BarMock.bind).toBe(mockGlobals.Function.prototype.bind);
});

it('does not mock methods from RegExp.prototype', () => {
Copy link
Contributor

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).

Copy link
Contributor Author

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/;

const barMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(bar),
);

expect(barMock).toBeInstanceOf(mockGlobals.RegExp);
expect(Object.prototype.hasOwnProperty.call(barMock, 'test')).toBe(false);
expect(barMock.test).toBe(mockGlobals.RegExp.prototype.test);
});

it('does not mock methods from RegExp.prototype (in mock context)', () => {
const bar = vm.runInContext(
`
const bar = /bar/;
bar;
`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird alignment again...

mockContext,
);

const barMock = moduleMocker.generateFromMetadata(
moduleMocker.getMetadata(bar),
);

expect(barMock).toBeInstanceOf(mockGlobals.RegExp);
expect(Object.prototype.hasOwnProperty.call(barMock, 'test')).toBe(false);
expect(barMock.test).toBe(mockGlobals.RegExp.prototype.test);
});

it('mocks methods that are bound multiple times', () => {
Expand Down
106 changes: 55 additions & 51 deletions packages/jest-mock/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,32 +226,6 @@ function isReadonlyProp(object: any, prop: string): boolean {
);
}

function getSlots(object?: Object): Array<string> {
const slots = {};
if (!object) {
return [];
}

let parent = Object.getPrototypeOf(object);
do {
if (object === Object.getPrototypeOf(Function)) {
break;
}
const ownNames = Object.getOwnPropertyNames(object);
for (let i = 0; i < ownNames.length; i++) {
const prop = ownNames[i];
if (!isReadonlyProp(object, prop)) {
const propDesc = Object.getOwnPropertyDescriptor(object, prop);
if ((propDesc !== undefined && !propDesc.get) || object.__esModule) {
slots[prop] = true;
}
}
}
object = parent;
} while (object && (parent = Object.getPrototypeOf(object)) !== null);
return Object.keys(slots);
}

class ModuleMockerClass {
_environmentGlobal: Global;
_mockState: WeakMap<Function, MockFunctionState>;
Expand All @@ -274,6 +248,53 @@ class ModuleMockerClass {
this._invocationCallCounter = 1;
}

_getSlots(object?: Object): Array<string> {
if (!object) {
return [];
}

const slots = new Set();
const EnvObjectProto = this._environmentGlobal.Object.prototype;
const EnvFunctionProto = this._environmentGlobal.Function.prototype;
const EnvRegExpProto = this._environmentGlobal.RegExp.prototype;

// Also check the builtins in the current context as they leak through
// core node modules.
const ObjectProto = Object.prototype;
const FunctionProto = Function.prototype;
const RegExpProto = RegExp.prototype;

// Properties of Object.prototype, Function.prototype and RegExp.prototype
// are never reported as slots
while (
object != null &&
object !== EnvObjectProto &&
object !== EnvFunctionProto &&
object !== EnvRegExpProto &&
object !== ObjectProto &&
object !== FunctionProto &&
object !== RegExpProto
mjesun marked this conversation as resolved.
Show resolved Hide resolved
) {
const ownNames = Object.getOwnPropertyNames(object);

for (let i = 0; i < ownNames.length; i++) {
const prop = ownNames[i];

if (!isReadonlyProp(object, prop)) {
const propDesc = Object.getOwnPropertyDescriptor(object, prop);

if ((propDesc !== undefined && !propDesc.get) || object.__esModule) {
slots.add(prop);
}
}
}

object = Object.getPrototypeOf(object);
}

return Array.from(slots);
}

_ensureMockConfig(f: Mock): MockFunctionConfig {
let config = this._mockConfigRegistry.get(f);
if (!config) {
Expand Down Expand Up @@ -336,7 +357,7 @@ class ModuleMockerClass {
metadata.members.prototype &&
metadata.members.prototype.members) ||
{};
const prototypeSlots = getSlots(prototype);
const prototypeSlots = this._getSlots(prototype);
const mocker = this;
const mockConstructor = matchArity(function() {
const mockState = mocker._ensureMockState(f);
Expand Down Expand Up @@ -606,7 +627,7 @@ class ModuleMockerClass {
refs[metadata.refID] = mock;
}

getSlots(metadata.members).forEach(slot => {
this._getSlots(metadata.members).forEach(slot => {
const slotMetadata = (metadata.members && metadata.members[slot]) || {};
if (slotMetadata.ref != null) {
callbacks.push(() => (mock[slot] = refs[slotMetadata.ref]));
Expand Down Expand Up @@ -678,7 +699,7 @@ class ModuleMockerClass {
// Leave arrays alone
if (type !== 'array') {
if (type !== 'undefined') {
getSlots(component).forEach(slot => {
this._getSlots(component).forEach(slot => {
if (
type === 'function' &&
component._isMockFunction &&
Expand All @@ -687,32 +708,15 @@ class ModuleMockerClass {
return;
}

if (
(!component.hasOwnProperty && component[slot] !== undefined) ||
(component.hasOwnProperty && component.hasOwnProperty(slot)) ||
(type === 'object' && component[slot] != Object.prototype[slot])
) {
const slotMetadata = this.getMetadata(component[slot], refs);
if (slotMetadata) {
if (!members) {
members = {};
}
members[slot] = slotMetadata;
const slotMetadata = this.getMetadata(component[slot], refs);
if (slotMetadata) {
if (!members) {
members = {};
}
members[slot] = slotMetadata;
}
});
}

// If component is native code function, prototype might be undefined
if (type === 'function' && component.prototype) {
const prototype = this.getMetadata(component.prototype, refs);
if (prototype && prototype.members) {
if (!members) {
members = {};
}
members.prototype = prototype;
}
}
}

if (members) {
Expand Down