Closed bcherny closed 7 years ago
Looks like you want protected internal
or private internal
. the internal modifier is tracked by https://github.com/Microsoft/TypeScript/issues/5228.
I should add that if these properties are truly private, then they should not be used by the tests.
on any rate, i do not think this is something we will be pursuing anytime soon. access modifiers in general is a complication for a structural type system. adding yet another layer of complexity around them would not be something we would want to do.
Looks like you want protected internal or private internal
That's not quite right. I want the members to remain private/protected from other source code's point of view (whether that source code is internal or external), but I want to give my tests special access to those members so they can be tested. To me, the point of ACL for members is to prevent erroneous usage, which as a side effect also prevents direct testing.
adding yet another layer of complexity around them would not be something we would want to do.
It's true that this is another layer, but I hope it is straight forward to treat everything as public
when the flag is set.
If you have to test private members of a class then that to me indicates that there is either an issue in the class design or in the design of the tests.
Could you provide an example?
@Jameskmonger I think you are conflating members being private and members being directly testable. Because something is private, does not mean I shouldn't be able to test it.
There is a unit testing philosophy that only public members should be tested, and that's usually ok. But sometimes there is a substantive piece of private code that deserves its own tests. It's a matter of taste, and I would love it if TSC was flexible enough to support this case.
Here's an example:
export class User {
constructor(private name: string, private email: string) {
}
isValid() {
return this.isNameValid() && this.isEmailValid()
}
private isNameValid() {
return /name_regex/.test(this.name)
}
private isEmailValid() {
return /email_regex/.test(this.email)
}
}
User
provides the isValid
method so consumers can check if a User is valid. It's probably a good idea to test the isNameValid
and isEmailValid
methods directly, but we can't because they are private! There are a few ways to deal with this and test the 2 methods today:
User
But these are all symptoms of TSC not being expressive, rather than concrete improvements on the existing design. In this example, private
is useful because it hides the 2 methods from consumers, so consumers only use the publicized API. But as a side effect, we are also hiding the methods from unit tests, which are not consumers in the same way that other source code is. To me this is a side effect of a weak member ACL, rather than a feature or indicator of code smell.
@bcherny a not cool but working solution could be to access those members dynamically. TS visibility modifiers are not compiled into special closures or anything like that. private
members are just there ready for use.
For example, you could:
var user = new User('some invalid name', '');
user['isNameValid']();
This is less than ideal but it works. Many drawbacks of dynamic code such as inability to refactor or being prone to typos are mitigated by the fact that these are tests. A typo or incorrect refactor should come up as a failed test.
@jods4 That is another option, though it look like it will stop working when TS2.1 is out. Another approach similar to one I've used in past Java applications is Reflect.get.
In any case, what I'm asking for is a blessed solution that works squarely with this use case, rather than a way to hack around the compiler's type resolver.
though it look like it will stop working when TS2.1
why? this should still be working.
@mhegazy I was misinterpreting this line:
Indexed access types of the form T[K], where T is some type and K is a type that is assignable to keyof T.
@bcherny This is new cool stuff that has to do with typing of code that uses dynamic property names, like being able to type a pluck
function:
function pluck<T, K extends keyof T>(obj: T, prop: K): T[K] {
return obj[prop];
}
let x = { a: 42 };
let y = pluck(x, 'a'); // statically verified that 'a' is in `x` and results in `y: number`.
x['something']
is the blessed way to perform dynamic access to properties, in fact there is even a compiler switch to allow it to return any
under --noImplicitAny
.
@jods4 Does dynamic property access bypass ACL by design? Is this something that will change in the future?
And that is a very cool feature. I was interpreting @ahejlsberg's writeup as saying that ACL will be enforced as well.
it is by design as an "escape" hatch. and will not be changed in the future.
but again, if you have something marked as private
, and you still need to access it, this is an indication that there is a design issue here that should be addressed. There is already affordance in the language to expose interfaces and keep implementations (classes) hidden; you can also have a public API module that exposes some types and not all, where as your tests have direct access to all your modules.
@mhegazy I would still prefer an explicit mechanism, but I'll use this as a next best workaround. How would you rewrite the example in my comment above? It seems heavy to use a facade for every class that has private methods.
this is a design decision based on your use cases and I can not claim that i understand your system to give advice.
That said, I would say if the only public property is isValid
then why bother testing the other two. testing isValid
with both invalid names and emails would be sufficient to ensure your class implements the contract correctly. which is rather the point of the test anyways. How it does it not interesting for the tester of class User
. at the end this is the whole point of encapsulation with OO patterns anyways, separation of concerns.
Assuming your logic in these functions is more complicated than just a regexp, and/or you use that in multiple places, I would have a Helpers
module, that is not exposed on the public API, and imported by User
containing module, that has two functions isValidName
and isValidEmail
and test these separately.
That said, I would say if the only public property is isValid then why bother testing the other two.
I'm not sure I agree with this - ACL often couples access from other code and access from unit tests, but the two don't always go together.
Assuming your logic in these functions is more complicated than just a regexp, and/or you use that in multiple places, I would have a Helpers module
That seems to be the cleanest way around it.
It's surprising to me that no one else has had this problem. I'll be curious to see if anyone else chimes in on this thread, and I'll use one of your solutions in the meantime. Thanks!
In your example @bcherny the way you'd test that would be to test isValid
with enough test cases that both isNameValid
and isEmailValid
are both covered.
You can test private methods through the public methods which interact with them - you should not be directly testing those private members alone.
You could do this:
test("given invalid name and invalid email, returns false", () => {
let user = new User("123", "fake_email");
Expect(user.isValid()).toBe(false);
});
test("given invalid name and valid email, returns false", () => {
let user = new User("123", "james@bla.com");
Expect(user.isValid()).toBe(false);
});
test("given valid name and invalid email, returns false", () => {
let user = new User("James", "fake_email");
Expect(user.isValid()).toBe(false);
});
test("given valid name and valid email, returns true", () => {
let user = new User("James", "james@bla.com");
Expect(user.isValid()).toBe(true);
});
The above tests will provide test coverage over your isValid
method, which is what you should be testing. You should not test private methods because there is no need to rely on their behaviour. They are an implementation detail of the class itself - test the behaviour and not the implementation.
Most of my unit tests test an interface's public members, but sometimes I want to test a private or protected method.
To do this today, I have to make the method public and add a JSDoc annotation ("Do not use!!!").
It would be nice to have a compiler flag (
--suppressControlledMemberAccessErrors
?) that I can use when compiling my code for unit testing, so I don't have to do gymnastics to test access controlled members.