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

Unable to use jasmine.spyOn on private/protected functions #14811

Closed
lukas-zech-software opened this Issue Feb 23, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@lukas-zech-software
Copy link
Contributor

lukas-zech-software commented Feb 23, 2017

  • 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]

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

lukas-zech-software pushed a commit to lukas-zech-software/DefinitelyTyped that referenced this issue Feb 23, 2017

@noomorph

This comment has been minimized.

Copy link
Contributor

noomorph commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Contributor Author

lukas-zech-software commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Contributor Author

lukas-zech-software commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Contributor

noomorph commented Feb 23, 2017

@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

This comment has been minimized.

Copy link
Contributor

noomorph commented Feb 23, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

lukas-zech-software commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Contributor

zhengbli commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Contributor Author

lukas-zech-software commented Feb 26, 2017

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

This comment has been minimized.

Copy link
Contributor

noomorph commented Feb 26, 2017

@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 referenced this issue Mar 4, 2017

@noomorph

This comment has been minimized.

Copy link
Contributor

noomorph commented Mar 10, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment