Suggestion: Add compiler flag to ignore private/protected member access control for unit tests

[bcherny]

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.

[mhegazy]

Looks like you want protected internal or private internal. the internal modifier is tracked by #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.

[bcherny]

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.

[Jameskmonger]

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?

[bcherny]

@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:

1. Make them public
2. Make them protected, and test them with a special test class that extends User
3. Pull them out into functions outside of the class and export them

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.

[jods4]

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

[bcherny]

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

[mhegazy]

though it look like it will stop working when TS2.1

why? this should still be working.

[bcherny]

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

[jods4]

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

[bcherny]

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

[mhegazy]

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.

[bcherny]

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

[mhegazy]

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.

[bcherny]

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!

[Jameskmonger]

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.