Unable to use jasmine.spyOn on private/protected functions

[lukas-zech-software]

• I tried using the @types/2.5.43 package and had problems.
• I tried using the latest stable version of tsc. TS v2.1.6
• [Mention]
  • Authors: @noomorph

Changes introduced in #14481 break functionality of jasmine as it is not longer possible to use spyOn functions in classes that are marked as private or protected

The keyof operator introduced in the PR can only see public methods
Details see this Typescript issue microsoft/TypeScript#13543

Solution is to bring back the un-generic version of spyOn and leave the generic version as opt-in overload

PR is coming shortly

[noomorph]

Is spyOn usage on private methods recommended? You could have used a cast ("as any") in the extreme cases when you badly need that, but why should we make it a default behavior?

[lukas-zech-software]

Recommend or not, it blocks actual jasmine functionality, which is something typings should never do.

If something works in Javascript in must also work in Typescript.

There is no other way than to make it the default again, as the function without generic parameter will always be the default if you do not explicitly add a generic parameter to your function call

spyOn(console,'log'); // --> declare function spyOn(object: any, method: string): jasmine.Spy;

spyOn<SomeType>(console,'log'); //  -->declare function spyOn<T>(object: T, method: keyof T): jasmine.Spy;

// This only works if there is no function with generic parameter declared at all
spyOn(console,'log');  // -->declare function spyOn<T>(object: T, method: keyof T): jasmine.Spy;

[lukas-zech-software]

It also does not only affect private or protected functions but also scenarios when a object gets additional functions during runtime with a decorator pattern or in setups where the objects do not use full type support (Typescript is a opt in language after all)

Also what does not work with the keyof operator is passing the value of the key as variable

const test='log';
spyOn(console, test);

All in all these changes broke a few of our tests that are totally valid in terms of Javascript runtime.
Therefore I think, your changes should be opt-in / optional in the cases where they actually make sense

[noomorph]

@lukas-zech-software, unfortunately I cannot agree with you regarding your previous answer (the next one I haven't read but soon I will).

Consider the behavior of spyOn in Jasmine when you are trying to execute the following:

	it('should fail here or not??', function () {
		spyOn({}, 'missingProperty');
	});

If you run this test, you are going to get the following error:

Error: missingProperty() method does not exist

That makes me think that by using TypeScript's keyOf feature we're actually preventing potential runtime errors in the build time, which also is a goal of TypeScript.

A bit off the topic, but actually I think we could go even further and make sure the value of the key is a function since that's what Jasmine demands.

Regarding your statement I can demonstrate another example:

If something works in Javascript in must also work in Typescript.

class Foo {
    private prop: string;
}

If I follow your logic which says that if I'm able to do that in JavaScript:

new Foo().prop = 'value';

then the TypeScript should not prevent me from doing that either, which is not the state of the things because it does prevent you from doing that.

Returning to the topic I'm saying that in reality nothing prevents you from modifying objects in a free-style manner, when you are using any (stating by that that you're taking a responsibility for the further consequences):

spyOn<any>(new Foo(), 'prop');
spyOn(new Foo() as any, 'prop');

I'll gladly listen to the opinions of the other maintainers and to your other arguments but personally I would not rush the things because
this needs a thorough thinking.

[noomorph]

@lukas-zech-software , I think that by adding any as a default type parameter we are actually going to lose more than to win in terms from the perspective of static type checking. As you said, it were just a few places in the code where you could easily fix that by adding as any or spyOn<any>. I had to go through the same painful thing in my project (5-7 files), but I didn't feel like this was something terribly wrong. If we compare how many times you'll need to add <any> in such cases and how many times you will be writing spyOn<MyAmazinglyBigClassName>, I think the latter is going to happen more often and pollute the code by its verbosity.

Regarding an example with a variable, it is really sad but still I think we need to collect more opinions from the community and maintainers. I'm saying I might be mistakening, but I tried also to list my considerations here in the thread as well.

I don't think there's a 100% right solution which will satisfy everyone, so we just need to agree which is going to be a default: T or any. As we see, you're on the side of any and I am on the side of T.

@zhengbli , what's your opinion about the issue?

[lukas-zech-software]

I see your point and I too am in favour off having as strong types as possible, actually any is a disallowed keyword in our codebase and needs an explicit linter exception.

What caused me to open this issue was, that the new version of the typing break code that is actually fine and working, by changing the default value from any to T

As I mentioned before, Typescript is opt-in, use as much typing as you like. It does not enforce the strictest typing possible and needs you explicitly reduce it too a level you're fine with.

That said, I can life with the changes but lets hear some other opinions on this. If no one lese follows my arguments, I'll happily close this PR.

[zhengbli]

I do agree that the non-generic version should be provided as an option. In TS 2.3 it will be possible to make a default value for the generic type parameter (microsoft/TypeScript#2175), but even with that the resulted use case should be the same: you have two options, to be stricter or not.

[lukas-zech-software]

After using the current typings and adapting my code accordingly I now see how bringing back the non-generic version complicates the usage of the generic function way to much to be reasonable.

In most cases you will want to use the generic version anyway, the need to explicitly set the generic parameter to get the benefit of it is a real annoyance.

Opting out of the typing by using spyOn<any> still seems to contradict the opt-in principle I pointed out above, but in this case I think the practical real word usage simply outweighs the rather theoretical principle.

Therefore I will close this issue and the PR.

If someone else sees this issue differently he or she may reopen this ticket, but as long as this doesn't happen, I'll assume that everyone ise more happy with the practical approach rather than with the 'academic' one 😉

[noomorph]

@Lukas-Zech, thanks and also I apologize that I haven't provided some meaningful examples for the problem. Back then I wanted to demonstrate that if we are going to add the any-version of spyOn, in 99% of cases the former generic version will just pass unnoticed because it won't be checking for the actual property existence (until you make a lot of effort to specify in every single case the verbose generic notation).

What is bad that I have no stats-based evidence that the generic version is okay in most cases. My only hope is that I provided enough other considerations that make sense.

Again, many thanks for the discussion.

[noomorph]

My musings on "why can't we use both declarations":

http://www.typescriptlang.org/play/index.html#src=%2F%2F%20The%20story%20of%20spyOn%3CT%3E%0D%0A%0D%0Aclass%20Foo%20%7B%0D%0A%20%20%20%20protected%20protectedFunction()%20%7B%0D%0A%0D%0A%20%20%20%20%7D%0D%0A%20%20%20%20public%20publicFunction()%20%7B%0D%0A%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0A%2F%2F%20haha%2C%20let's%20try%20out%20strict%20spyOn%0D%0A%0D%0Adeclare%20function%20hard_spyOn%3CT%3E(object%3A%20T%2C%20method%3A%20keyof%20T)%3A%20void%3B%0D%0A%0D%0Ahard_spyOn(new%20Foo()%2C%20'publicFunction')%3B%20%2F%2F%201.%20cool%2C%20it%20works%0D%0Ahard_spyOn(new%20Foo()%2C%20'nonexistingFunction')%3B%20%2F%2F%202.%20perfect%2C%20it%20prevented%20a%20bug%0D%0Ahard_spyOn(new%20Foo()%2C%20'protectedFunction')%3B%20%2F%2F%203.%20what%3F!%20why%20can't%20I%20spy%20on%20non-public%20function%3F%0D%0A%0D%0A%2F%2F%20let's%20add%20an%20overload%2C%20huh%3F%0D%0A%0D%0Adeclare%20function%20soft_spyOn(object%3A%20any%2C%20method%3A%20string)%3A%20void%3B%0D%0Adeclare%20function%20soft_spyOn%3CT%3E(object%3A%20T%2C%20method%3A%20keyof%20T)%3A%20void%3B%0D%0A%0D%0Asoft_spyOn(new%20Foo()%2C%20'publicFunction')%3B%20%2F%2F%201.%20cool%2C%20we%20did%20not%20break%0D%0Asoft_spyOn(new%20Foo()%2C%20'protectedFunction')%3B%20%2F%2F%202.%20amazing%2C%20we%20can%20spy%20on%20protected%20functions%0D%0Asoft_spyOn(new%20Foo()%2C%20'nonexistingFunction')%3B%20%2F%2F%203.%20oh%20no!%20it%20stopped%20checking%20at%20all%2C%20it%20is%20useless%20%0D%0A%0D%0A%2F%2F%20things%20to%20think%20of%0D%0A%0D%0Anew%20Foo().protectedFunction()%3B%20%2F%2F%20we%20don't%20complain%20when%20this%20does%20not%20work%0D%0A(new%20Foo()%20as%20any).protectedFunction()%3B%20%2F%2F%20instead%20we%20try%20something%20like%20that%0D%0A%0D%0A%2F%2F%20so%20what%20do%20we%20do%3F%0D%0A%0D%0Ahard_spyOn%3Cany%3E(new%20Foo()%2C%20'protectedFunction')%3B%20%2F%2F%20let's%20apply%20the%20same%20workaround%20for%20private%20and%20protected%0D%0A%0D%0A%2F%2F%20Thanks%20for%20your%20attention

[ursaj]

this problem is really annoying. we have a lot of tests relying on spy(s) of protected/private methods. I'v ended up with the following:

declare global {
    function spyOn<T>(object: T, method: keyof T): jasmine.Spy;
    /** @deprecated Consider to grant 'public' access level for spied method. */
    function spyOn<T>(object: T, method: string): jasmine.Spy;
}

could you please make it a part of jasmine typings? it should make both parties happy:

• those, who need only 'public' (keyof)
• and those, who need full access to all methods (original behavior)