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.

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

Typescript 4.4.2 bug #17

Closed

Conversation

johanblumenberg
Copy link

@johanblumenberg johanblumenberg commented Jul 25, 2022

#15

Problem

From this thread: NagRock#212 (comment)

I created a test repo with a bunch of spec files that highlight the issue. If you'd like to take a look, it's here.

I tried the test BasicClass > mocking combineLatest observables on service under test > should not mock for local observables in combineLatest with no pipe, and I think that the problem is the MocableFunctionsFinder class in ts-mockito.

It looks like typescript changed how it generates code. There is a difference in how it generates the BaseClass code. These are the interesting bits:

Typesscript 4.3.5:

        /* istanbul ignore next */
        cov_28xc3y53ju().s[15]++;
        this.localCombinedObservableNoPipe$ = rxjs_1.combineLatest([this.observable$, this.observableTwo$]);

Typescript 4.4.2:

        /* istanbul ignore next */
        cov_28xc3y53ju().s[15]++;
        this.localCombinedObservableNoPipe$ = (0, rxjs_1.combineLatest)([this.observable$, this.observableTwo$]);

This MocableFunctionsFinder class tries to figure out all member functions in a class, by looking at the source code of the class constructor. The regex matching described on this line will have a problem with the new typescript generated code:

* - [.]functionName = (

Solution

I think that trying to parse the constructor code to figure out any property that is set, and that is set to a function, is really error prone. You would need a full JavaScript parser to do it properly, I don't know if it is possible to fix the current regex solution to handle this case.

I think a better solution is to always use a Proxy object always.

Looking at this code here:

constructor(private clazz: any, public instance: any = {}) {

The code tries to determine if it is mocking a class or an interface. If it is mocking a class it tries to find all properties and methods in advance, by inspecting the prototype chain and examining the code of various constructors and functions. If it is mocking an interface there is no class where you can search for properties, so instead it uses a Proxy object to intercept all calls in "runtime", and creates all the mocked properties on the fly.

I think it would be better to always use a Proxy object. Like how it is done in my fork, here: https://github.com/johanblumenberg/ts-mockito/blob/e5c009f9df45e4141744aaa92e4481735fd649b3/src/Mock.ts#L38

In my fork there is no difference between mocking a class or an interface. I tried to comment out the same lines as in this PR, and it still works, because the call is intercepted in runtime, just like for interfaces.
In this repository the proxy objects are set up differently for interfaces and classes, and when mocking a class it requires all properties and methods to be set up in advance. So some of the tests break.

All major browsers and platforms do support the Proxy object by now, except Internet Explorer. So I think it might make sense to remove the code that walks the prototype chain and tries to figure out the shape of the class in advance, and only use the code that intercepts the call and creates the mock properties in "runtime".

@johanblumenberg
Copy link
Author

johanblumenberg commented Jul 25, 2022

I can port relevant parts of my fork to solve the problem. Question is if to support environments where Proxy is not available.

A compromise might be to remove the parsing of constructor code and only support methods created dynamically if Proxy is available, but keep the simple walking of prototype chain to figure out properties and methods.

@LironHazan
Copy link
Collaborator

@johanblumenberg amazing while you were writing this: " full JavaScript parser" I was playing with the swc parser plugin system :)
I agree that the regex isn't a good candidate here, especially when trying to pluck an arrow function name,

The bellow code works, but I didn't test any usecase so I'll leave that for the moment and use your fix! thanks!

// https://swc.rs/docs/usage/plugins#visitor-api

const classAsTranspiledString = `
"use strict";
var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
    function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
};
var __generator = (this && this.__generator) || function (thisArg, body) {
    var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g;
    return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
    function verb(n) { return function (v) { return step([n, v]); }; }
    function step(op) {
        if (f) throw new TypeError("Generator is already executing.");
        while (_) try {
            if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t;
            if (y = 0, t) op = [op[0] & 2, t.value];
            switch (op[0]) {
                case 0: case 1: t = op; break;
                case 4: _.label++; return { value: op[1], done: false };
                case 5: _.label++; y = op[1]; op = [0]; continue;
                case 7: op = _.ops.pop(); _.trys.pop(); continue;
                default:
                    if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; }
                    if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; }
                    if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; }
                    if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; }
                    if (t[2]) _.ops.pop();
                    _.trys.pop(); continue;
            }
            op = body.call(thisArg, _);
        } catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; }
        if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true };
    }
};
exports.__esModule = true;
exports.Foo = void 0;
var Foo = /** @class */ (function () {
    function Foo() {
        this.dynamicMethod = function () { return "dynamicMethod"; };
    }
    Foo.prototype.getBar = function () {
        this.dynamicMethodInFunction = function () { return "dynamicMethodInFunction"; };
        return "bar";
    };
    Foo.prototype.concatStringWithNumber = function (sampleString, sampleNumber) {
        return sampleString + sampleNumber;
    };
    Foo.prototype.convertNumberToString = function (value) {
        return value.toString();
    };
    Foo.prototype.getStringById = function (value) {
        return value.toString();
    };
    Foo.prototype.sumTwoNumbers = function (a, b) {
        return a + b;
    };
    Foo.prototype.sampleMethodWithOptionalArgument = function (a, b) {
        return a + b;
    };
    Foo.prototype.sampleMethodWithTwoOptionalArguments = function (a, b) {
        return a + b;
    };
    Foo.prototype.sampleMethodReturningPromise = function (value) {
        return Promise.resolve(value);
    };
    Foo.prototype.sampleMethodReturningVoidPromise = function (value) {
        return Promise.resolve();
    };
    Foo.prototype.sampleMethodReturningVoidPromiseWithoutParams = function () {
        return __awaiter(this, void 0, void 0, function () {
            return __generator(this, function (_a) {
                return [2 /*return*/, Promise.resolve()];
            });
        });
    };
    return Foo;
}());
exports.Foo = Foo;

`;

const Visitor = require('@swc/core/Visitor.js').Visitor;
const swc = require('@swc/core');

const names = [];
class ExVisitor extends Visitor {
  visitFunction(n) {
    !!n.identifier?.value && names.push(n.identifier.value);
    return super.visitFunction(n);
  }
  visitAssignmentExpression(n) {
    if (n.right?.type === 'FunctionExpression') {
      n.left?.type === 'MemberExpression' && names.push(n.left.property.value);
    }
    return super.visitAssignmentExpression(n);
  }
}

class Transformer {
  static run() {
    const result = swc.transformSync(classAsTranspiledString, {
      plugin: (program) => new ExVisitor().visitProgram(program),
    });
    console.log('names', names);
  }
}

Transformer.run();

@pauleustice
Copy link

@johanblumenberg Thanks so much for your input and explanation. I agree that using a regex rather than an AST for that stuff isn't great...

One question though - if you are always assigning the Proxy to whatever you're spying on, doesn't that just mock out the whole spied class? i.e. you would need to assign returns for all class fields, methods etc on your spy? That's definitely a significant change to the test suites in our codebase.

Proxy is available on almost all browsers; I don't think anyone is running test suites on IE or Opera Mobile :)

@LironHazan
Copy link
Collaborator

@johanblumenberg I don't think there would be a modern runtime that won't support Proxy, AFAIK edge vendors such as cloudflare and deno are aligned with the browser and aim to keep one standard so let's port your code for making the PR work?

@johanblumenberg
Copy link
Author

One question though - if you are always assigning the Proxy to whatever you're spying on, doesn't that just mock out the whole spied class?

@pauleustice I think that the spy just forwards all calls to the original method instead of forwarding to a mock, but I don't remember the details.

@LironHazan
Copy link
Collaborator

One question though - if you are always assigning the Proxy to whatever you're spying on, doesn't that just mock out the whole spied class?

@pauleustice I think that the spy just forwards all calls to the original method instead of forwarding to a mock, but I don't remember the details.

@pauleustice the original methods seem to be called

@LironHazan
Copy link
Collaborator

@johanblumenberg @pauleustice I opened a PR with a temp fix, LMK what you think
https://github.com/TypeStrong/ts-mockito/pull/18/files

@johanblumenberg
Copy link
Author

@LironHazan @pauleustice @cspotcode I have played around with ts-mockito a bit. This is what I found.

Mocking

A ts-mockito Mocker object needs to know the type of all properties on an object that is mocked. It needs to know if it is a method or a property, to return a function that returns the actual mocked value or directly return the actual mocked value.

In some cases ts-mockito can figure out the type of the property by inspecting the object to spy on or the class to mock.

If ts-mockito cannot figure out the type, you can tell the mocked object what type a property is by setting up an expectation, such as when(foo.x()).thenReturn(...). If no expectation is set up, ts-mockito must guess the type.

Mocking interfaces

All properties are mocked as methods. If you set up an expectation you can declare specific properties as properties.

Mocking classes

ts-mockito has two strategies for figuring out the types of the properties. It will inspect the prototype chain to find all methods, and it will also parse the constructor code to find any assigned method.

The mock relies on being able to find all methods, and then assumes that any other property is a property.

One drawback with the current implementation is that if it doesn't find a particular method, it will assume that it is a property, and it is not possible to correct this by adding an expectation. (bug 1)

There are two other bugs in the constructor parsing as well. The first bug is that it thinks that a property that starts with a ( is a method (bug 2). The other bug is that it thinks that any method that doesn't start with function or ( is a property (bug 3).

Spies

The current implementation of spies works just like mocking classes, but it also inspects all properties on the spied on object, to find additional methods.

There is a limitation that when spying on properties, it is not possible to set expectations. (bug 4)

There is a limitation that spying on functions that are assigned in the constructor does not work. The descriptor in the real function is undefined from trying to get the value from the prototype chain. (bug 5)

Class constructor parsing

Originally I thought that you can just remove the class constructor parsing, because I thought that you will find all methods by walking the prototype chain of the class, and any method that is not found can be set by adding an expectation, just as it works with interfaces.

This proved to be wrong, in two ways.

The first reason is that class mocking relies on that the mock can figure out all methods, and all other properties are assumed to be properties. So unlike class mocking, you cannot correct any missing methods by adding an expectation.

The other reason is that it is not a spacial case that a method is declared in a class constructor, and not in the code. The typescript compiler generates this type of code for methods declared as arrow functions. They are not put in the prototype chain. This means that removing the constructor code parsing will break a lot of tests that are mocking such functions, and require you to add expectations to correct the type.

This means that it is probably not possible to remove the constructor code parsing.

Commits

Test reproducing bug /issues/15

Adding tests to reproduce the bug, and a few other related cases.

Failed tests:

Dynamic Methods > Mocking a class › should return the mocked value when an expectation is set, assigned function TS435 - Failing because of (bug 3), the method is wrongly typed as a property, and because (bug 1), correcting the type using an expectation does not work on class mocks.

Dynamic methods > Mocking a class › should return null when no expectation is set, assigned function TS435 - Failing because of (bug 3), the method is wrongly typed as a property.

Dynamic methods › Spying on an object › should return original value when no expectation is set, arrow function - Failing because of (bug 5), not possible to mock assigned methods on a spy.

Dynamic methods › Spying on an object › should return original value when no expectation is set, assigned function TS442 - Failing because - Failing because of (bug 5), not possible to mock assigned methods on a spy.

Dynamic properties › Mocking a class › should return null when no expectation is set, TS442 - Failing because of (bug 2), the property is wrongly typed as a method.

Dynamic properties › Spying on an object › should return the mocked value when an expectation is set. TS442 - Failing because of (bug 4), not possible to set expectations on properties on spies.

Dynamic properties › Spying on an object › should return original when no expectation is set, TS442 - Failing because of (bug 3), the method is wrongly typed as a property, and because (bug 4), not possible to set expectations on properties on spies.

Unify mocks for classes and interfaces

Refactor mock creation slightly, but no change of functionality. Create mocks in the same way for classes, interfaces and spies. The only difference is that when mocking interfaces, properties are assumed to be methods by default.

Fix methods wrongly assigned as properties

Fixes (bug 1).

This commit means that it is always possible to fix the type by adding an expectation, if ts-mockito guesses the type wrong.

Fixes Dynamic Methods > Mocking a class › should return the mocked value when an expectation is set, assigned function TS435, it is now possible to correct a wrongly typed function by adding an expectation.

Breaks Dynamic properties › Spying on an object › should return the mocked value when an expectation is set, TS435, because it is now possible to correct the wrongly typed property, and then it fails because of (bug 4), not possible to mock properties on a spy.

No class parsing for spies

Fixes (bug 5), and fixes (bug 2) and (bug 3) for spies.

This avoids (bug 5) because the prototype chain is not walked in spies any more. It is not needed for spies, since you can get all properties from the object itself.

Fixes Dynamic methods › Spying on an object › should return original value when no expectation is set, arrow function, Dynamic methods › Spying on an object › should return original value when no expectation is set, assigned function TS442.

Remaining failures

The remaining bugs are (bug 2), (bug 3), (bug 4).

(bug 2) and (bug 3) must probably be fixed by fixing the constructor parsing code.

After these changes:

  • It is always possible to correct a wrongly guessed type by adding an expectation.
  • Everything related to methods on a spy works.
  • There is still a bug that you cannot mock properties on a spy.
  • The constructor parsing code still has bugs but they do not affect spies any more.

@johanblumenberg
Copy link
Author

johanblumenberg commented Jul 31, 2022

@LironHazan About the code you posted here #17 (comment)

After playing around with the typescript compiler a bit, I think a proper parser is needed to fix the issues that surfaced with typescript 4.4.2.

Will your code work with any expression returning a function, or just function expressions? So it catches any member assignment on the form this.fn = function () {...}, but does it also catch anything that returns a function, such as this.fn = _.debounce(...)? Is it type aware?

Would it be an idea to use typescript itself to parse the constructor code? Something like this: #17 (comment)
All users of ts-mockito probably already have typescript as a dependency. It could perhaps be added as a peer or optional dependency.

@johanblumenberg johanblumenberg marked this pull request as ready for review July 31, 2022 23:55
@johanblumenberg
Copy link
Author

@LironHazan After digging a bit deeper (see above), it might make sense to keep the constructor parsing, and then your fix here also makes a lot of sense: #17 (comment)

I tried a different approach in this PR, to add another subclass of Mocker for mocks. wdyt?

@LironHazan
Copy link
Collaborator

@LironHazan @pauleustice @cspotcode I have played around with ts-mockito a bit. This is what I found.

@johanblumenberg Thanks for the effort!! I'll deeply read that and response

@LironHazan
Copy link
Collaborator

@LironHazan About the code you posted here #17 (comment)

After playing around with the typescript compiler a bit, I think a proper parser is needed to fix the issues that surfaced with typescript 4.4.2.

Will your code work with any expression returning a function, or just function expressions? So it catches any member assignment on the form this.fn = function () {...}, but does it also catch anything that returns a function, such as this.fn = _.debounce(...)? Is it type aware?

@johanblumenberg my code only covers part of the cases, I guess there's a need to implement other visitors functions to cover all cases. (found here --> https://github.com/swc-project/swc/blob/main/node-swc/src/Visitor.ts)
Regards "type awareness" if I understand you correctly, swc is a typescript/javascript transpiler so it awares of
types But - when I debugged ts-mockito I've noticed that the code which passed as a string is a transpiled one which makes sense when in runtime

Would it be an idea to use typescript itself to parse the constructor code? Something like this: #17 (comment) All users of ts-mockito probably already have typescript as a dependency. It could perhaps be added as a peer or optional dependency.
@johanblumenberg I assume tsc can parse javascript so we can use it but I personally like working with the ts-morph abstraction of it, but using swc will give us better performance from other hand, but for our usecase which is only parsing a single string at a time the performance aspect isn't relevant.. I'll open an issue for adding a parsing layer and we can discuss that :)

@johanblumenberg
Copy link
Author

when I debugged ts-mockito I've noticed that the code which passed as a string is a transpiled one

What I'm after is this case:

class Foo {
  public x = someFunction();
}

How do you know what someFunction() returns, is it a function or a property?

A real world scenario that might be quite common would be to use _.debounce() or something similar to create public methods.

If you could use a type aware parser, it would know the return type of _.debounce(), and you could just ask the AST what type the expression returns. However, since you only get the code snippet from the constructor function, you don't get any import statements or anything else, it might be impossible to achieve full type support.

@johanblumenberg
Copy link
Author

johanblumenberg commented Aug 1, 2022

Maybe the best approach to parsing is something really simple and well documented. For example like this:

We make sure that the parsing handles common typescript generated code, such as the ones we have identified. Assignment of function member in constructor, which is generated when you use an arrow function as a method. Maybe we can keep some simple regex parsing for this scenario, to avoid large dependencies.

We ignore other strange constructs, for example if you create a method by invoking another function, because of the complexity of parsing.

We document clearly how to detect and handle other cases. You need to add a when(foo.x()) statement to fix the type of x, before you use the mock instance object.

I think this approach: simple and correct with documented behaviour of where it will get the types right and when you need to add a when() statement, is better than the alternative: complex parsing that is sometimes getting the types wrong, and which is hard to explain to users.

@LironHazan
Copy link
Collaborator

Maybe the best approach to parsing is something really simple and well documented. For example like this:

We make sure that the parsing handles common typescript generated code, such as the ones we have identified. Assignment of function member in constructor, which is generated when you use an arrow function as a method. Maybe we can keep some simple regex parsing for this scenario, to avoid large dependencies.

We ignore other strange constructs, for example if you create a method by invoking another function, because of the complexity of parsing.

I agree

We document clearly how to detect and handle other cases. You need to add a when(foo.x()) statement to fix the type of x, before you use the mock instance object.

Sounds reasonable, I rather keep it simple as well,

I think this approach: simple and correct with documented behaviour of where it will get the types right and when you need to add a when() statement, is better than the alternative: complex parsing that is sometimes getting the types wrong, and which is hard to explain to users.

Mixing of mocking and stubbing is fair IMO, but it's also worth checking jest, es6-class-mocks or other implementations, maybe we'll get a new angle

@pauleustice
Copy link

Amazing work @johanblumenberg, and sorry for the late reply. Thankful for such a comprehensive investigation and clear write up!

Regarding bug 4 (and the test Dynamic properties › Spying on an object › should return the mocked value when an expectation is set), isn't that intended behaviour? To me that's the intentional delineation between spies and mocks.

Obviously everyone's needs and use-cases are different, but I have written tonnes of ts-mockito tests on complex Angular apps and never needed to mock a value on a spy. Can you think of any examples where it'd be useful?

@johanblumenberg
Copy link
Author

johanblumenberg commented Aug 5, 2022

Regarding bug 4 (and the test Dynamic properties › Spying on an object › should return the mocked value when an expectation is set), isn't that intended behaviour? To me that's the intentional delineation between spies and mocks.

Obviously everyone's needs and use-cases are different, but I have written tonnes of ts-mockito tests on complex Angular apps and never needed to mock a value on a spy. Can you think of any examples where it'd be useful?

I agree that setting property values on a spy might not be very useful. But if I have added the statement when(foo.calculatedPropertyTS435).thenReturn("value");, then I would expect the spy to return my value on the property.

I'm not sure we need to fix this bug, since it is not very useful. Perhaps a better approach is to throw an exception if you try to change a property value on a spy, to say that this operation is not supported?

@pauleustice
Copy link

I agree that setting property values on a spy might not be very useful. But if I have added the statement when(foo.calculatedPropertyTS435).thenReturn("value");, then I would expect the spy to return my value on the property.

I'm not sure we need to fix this bug, since it is not very useful. Perhaps a better approach is to throw an exception if you try to change a property value on a spy, to say that this operation is not supported?

I guess it's a decision between updating documentation (to explicitly mention that it isn't supported), or throwing if someone does try to do it. Either one (or both) will be better than failing silently, which is where so much testing pain comes from, in my experience :)

@pauleustice
Copy link

@LironHazan @johanblumenberg Any progress on whether we can merge this and release to npm?

Am I right in thinking it is only the constructor parsing regex (or alternative AST approach) that remains to solve bugs 2 and 3?

…t-as-function-issue"

This reverts commit 1e2cd43, reversing
changes made to 426058d.
@johanblumenberg
Copy link
Author

@pauleustice

Any progress on whether we can merge this and release to npm?

I rebased it, so it is ready to be merged

Am I right in thinking it is only the constructor parsing regex (or alternative AST approach) that remains to solve bugs 2 and 3?

Bug 2 can probably be fixed by updating the constructor parsing.

Bug 3 might be difficult, since if a function is assigned from the return value from another function, it is not really possible to know if it is a method or a property, depending on what that function returns. So in those cases it might be easier to just document the behaviour. If you do weird stuff, you need to help out by adding when(obj.x())... to tell ts-mockito the type of the property.

@LironHazan
Copy link
Collaborator

@LironHazan @johanblumenberg Any progress on whether we can merge this and release to npm?

Am I right in thinking it is only the constructor parsing regex (or alternative AST approach) that remains to solve bugs 2 and 3?

Sorry for the delay - I already published a fix to the initial issue which skips mocking spies,
@pauleustice can you check that it works for you? version 2.6.3

@pauleustice
Copy link

pauleustice commented Aug 17, 2022

@LironHazan Just tried it, and all my tests pass! Amazing, thank you so much (and @johanblumenberg) for your collaboration on this!

@LironHazan
Copy link
Collaborator

Closing

@roypeled
Copy link
Collaborator

roypeled commented Jul 7, 2024

Hey @johanblumenberg, I'm kind of late to the party, but let me know what you think of my PR replacing the regex parser with an actual parser: #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants