Unit Testing

[kirlat]

This issue is for discussion of our approach to unit testing. The discussion initially started at https://github.com/alpheios-project/components/pull/318#issuecomment-455052187

@kirlat was saying:

Eases testing as probably the only thing we need to test of a module are public API and a store. The rest is not exposed and should not be unit tested (that's debatable but I think this is what our current approach is and I agree with it). Are we going to stop using unit tests? Or it would be dependent on developer's choice? I am using unit tests - as the stage of updating code and as the first stage for creating code (it is easier to create and test method from unit test perspective rather than from whole application perspective)

We were discussing the unit test approach with @balmas and we agreed that, in general, only public functions of the object should be tested (Bridget, please correct me if I'm wrong here). The idea behind this is that each object can be considered as a black box. It has a public interface: a set of methods that are called by other objects. It also has a private interface: service methods that are called only by methods of the same object. Unfortunately, this separation is mental only right now as it is not supported on the language level, but I hope it will get there finally: https://github.com/tc39/proposal-private-methods#private-methods-and-fields. So far we can start names of private methods and fields with _ to signify that they should not be used from the outside. If the proposal mentioned above will be approved, we can replace _ with # easily.

In unit tests we need to verify that an object, being a black box as it is and given some inputs (i.e. have some methods called with certain set of argument values) should provide correct output (i.e. return correct values or produce proper state changes). If only public methods are called from the outside, then we should test public methods only. No matter what other private (internal) methods do and how do they behave exactly: as long as they do not affect the output of the public functions, we do not care (as all we care is the output of the public functions). That will give use some freedom to fiddle with object's internal structure without updating unit tests every time something inside the object is changed (as long as it does not change public methods and their output).

Does it make sense? What do you think?

@irina060981 was saying:

I was looking at unit tests from another perspective. Unit tests are tests for little parts of the code (that's why they are called unit). And they have several usage purposes:

1. "Starting from tests" - I ussually starting to develop and check the code creating simple tests.
   For example, I am working with new concordance adapter.
   I need implement several features - geting data from API, parse data, upload to data-models objects
   Insted of creating a large amount of code at once (to be able to check it from LexicalQuery), I am creating a step by step and check it inside unit tests without builfing and importing into another library

2. "Creating overal tests" - after creating first working prototype - I am creating unit tests for each methods and while doing it I find unneeded methods, unuseful parts of the code, I add additional checks, and make different refactoring - because such test coverage allows to see all over the picture

3. "Describing code workflow" - after some time - when I return to the code or read other's developer code - I could read test's description and see what arguments are waited to be passed by some user's scenario

4. "Look at the time spent for some operations" - tests allow to calc time need for some operations, api retreiving.

5. "Check the difference that was made to the code by another developer comparing tests" (similiar to 3) )

6. And only the last is checking current code quality

But as I could see from your text you are using tests only for the last point. Then it seems to me that they are not very useful for you and don't cost time needed for its updating :)

[balmas]

(Moved from #318)

We were discussing the unit test approach with @balmas and we agreed that, in general, only public functions of the object should be tested (Bridget, please correct me if I'm wrong here).

Actually this isn't quite right. I think integration tests (i.e. tests which test how one piece of code interacts with another) should focus on the public functions of the objects. However unit tests should test all the business logic within a class, including private methods if necessary.

Sorry for misunderstanding and thanks for correction! 🙂

[balmas]

(moved from #318)

I was looking at unit tests from another perspective. Unit tests are tests for little parts of the code (that's why they are called unit). And they have several usage purposes:

1. "Starting from tests" - I ussually starting to develop and check the code creating simple tests. For example, I am working with new concordance adapter. I need to implement several features - geting data from API, parse data, upload to data-models objects. Instead of creating a large amount of code at once (to be able to check it from LexicalQuery), I am creating a step by step and check it inside unit tests without building and importing into another library Also it is really easy to create mock parts for any step that make code much simplier. And it makes me think how to make methods short to make tests simplier :)
2. "Creating overal tests" - after creating first working prototype - I am creating unit tests for each method and while doing it I find unneeded methods, unuseful parts of the code, I add additional checks, and make different refactoring - because such test coverage allows to see all over the picture.
3. "Describing code workflow" - after some time - when I return to the code or read other's developer code - I could read test's description and see what arguments are waited to be passed by some user's scenario.
4. "Look at the time spent for some operations" - tests allow to calc time need for some operations, api retreiving.
5. "Check the difference that was made to the code by another developer comparing tests" (similiar to 3) )
6. And only the last is checking current code quality

I think this is a very good demonstration of test-driven development. I agree that this is a good methodology to try to follow and at the same time confess that I find it takes a huge amount of discipline to switch to it if you have coded the opposite way for a long time. Nonetheless I agree we should be aiming for this whenever possible.

[irina060981]

Thank you, Bridget :) I am really happy to read such a feedback on my approach :)

[kirlat]

I was not following the TDD approach for all parts of Alpheios apps. Mostly that's because when system is developing fast and its architecture is not stabilized yet (as we had with some libraries of an Alpheios project), it requires a lot of effort to constantly update the tests and slows down a progress significantly. One could end up wasting a lot of time writing tests for features that are quickly dropped.

But once the architecture is stabilized, I do agree that TDD is all about benefits, not drawbacks. If can even help to find some flaws in an architecture. I would really love to stick to it.

[kirlat]

@irina060981 Kudos for following the TDD!

[irina060981]

@kirlat, thank you :)

[balmas]

(Moved from #318)

We were discussing the unit test approach with @balmas and we agreed that, in general, only public functions of the object should be tested (Bridget, please correct me if I'm wrong here).

Actually this isn't quite right. I think integration tests (i.e. tests which test how one piece of code interacts with another) should focus on the public functions of the objects. However unit tests should test all the business logic within a class, including private methods if necessary.

Sorry for misunderstanding and thanks for correction!

After spending a few days writing tests, I wanted to offer a little clarification on this.

Take the following pseduo code:

updateItem() {
  //first get the item
 my item = _privateGetItem()
 item.isUpdated = true
 return item.isUpdated
}

getItem() {
 my item = _privateGetItem()
 return item
}

_privateGetItem() {
}

In this case, both public methods updateItem and getItem call the internal private method _privateGetItem to retrieve the item they are asked for. In this case, it may not be necessary to write a unit test for _privateGetItem because this functionality will be exercised and tested by tests for updateItem and getItem. There may be situations where it's still a good idea to write a test for a private method, particularly if there is logic that could be wrong. In the above example, if _privateGetItem did a complex calculation to populate some attribute of the item it returns, that isn't explicitly tested elsewhere, then a unit test on this method might be warranted.

[irina060981]

About the following code there could be another point of view. As unit tests should test only one method in one test, then sometimes it is useful to to test with mock to avoid missing changes in inner functions for example for the current pseudo-code:

PseudoClassInstance._privateGetItem = jest.on(()=>{ return 5 })

you could use this mock in updateItem and getItem method, so you will test then the code inside the current method and the result of your testing the current method won't be changed even if sometime you will need to change _privateGetMethod.

Also one more advantage here you could really see what exactly inputs and outputs could be used here and what checks for arguments you could need.

But anyway I think that sometimes such approach is useful, sometimes is not.

[balmas]

that's an interesting perspective too. I agree that could well be a useful approach sometimes.

[kirlat]

To unit test private methods or not to test, that’s the question, as one author put it 🙂. Or, also, what would we do about white box testing vs. black box testing vs. gray box testing approaches? That is a question too. 🙂.

I'm not a testing expert in any way, but a decision not to unit test private methods was more acceptable logically for me. That's why it was my initial reaction in a discussion. A private method is an implementation detail that should be hidden to the users of the class. Testing private methods breaks encapsulation.

The private methods on a class should be invoked by one or more of the public methods (perhaps indirectly - a private method called by a public method may invoke other private methods). Therefore, when testing your public methods, you will test your private methods as well. If you have private methods that remain untested, either your test cases are insufficient and a set of tests should be expanded or the private methods (or parts of private methods functionality that is untested) are unused and can be removed. That's an easy concept for me to be set on.

By coupling tests to private implementation details, a unit test suite can become fragile. This can lead to false positives during refactoring. That's what I experienced quit often, and that can slow down refactoring a lot.

There is even an opinion that testing a private method directly is a good indicator of an SRP (single responsibility principle) violation. One option in that case, and often the easiest if the abstraction makes sense, is to perform an extract class refactoring. I feel that this, however, can be a too radical approach at times.

On the other hand, there could be good reasons to do test private methods. On my opinion this is either if a set of tests to verify all aspects of business logic through public methods is becoming too cumbersome, or if there is some special logic in private methods that can be verified with direct testing only.

I've checked several source on the Internet, and many of them are wonderful. Majority of authors think that private methods should generally not be tested, but opinions do differ. Here is a set of articles on the subject I found particularly interesting: Should Private Methods Be Tested? https://codeahoy.com/2016/11/19/should-you-unit-test-private-methods/ Unit testing private methods https://enterprisecraftsmanship.com/2017/10/23/unit-testing-private-methods/ On testing private methods https://medium.com/stuart-engineering/on-testing-private-methods-34109c90a72a Why shouldn't I test private methods? https://dzone.com/articles/why-shouldnt-i-test-private How to avoid the need to Unit test private methods https://softwareengineering.stackexchange.com/questions/375860/how-to-avoid-the-need-to-unit-test-private-methods

I think we should find an approach that fits our architecture best. It's probably both art and science 🙂. What do you think?

[kirlat]

When writing unit tests, I found it beneficial to follow the approach of only one thing (one use case of one method) to be tested at a time. In that case if test fails, one can know what fails exactly.

If several assertions are grouped into one test, and if any of them, or even several ones at once fail, it will be harder to find where the problem is at a glance. We'll get an overall negative, and it will require us to figure out what is to blame.

However, if each single test involves high setup costs (i.e. creation of objects to test with is lengthy) it might be OK to group several assertions together. That will help to reduce an overall test suite run time.

What are your approaches and experiences with this?

[kirlat]

I stumbled upon some extensive set of no-nonsense practices for JS testing. I would like to share it with you: https://medium.com/@me_37286/yoni-goldberg-javascript-nodejs-testing-best-practices-2b98924c9347

[irina060981]

Thank you, it would be interesting to read it!

[balmas]

thanks!

[balmas]

this is funny from that article: "Otherwise: The team will write less test and decorate the annoying ones with .skip()"

Will think of it when I add skip to a test :-)

[balmas]

Another comment on the article, more serious this time, it is really easy to slip into white-box vs black-box testing. This is one of the things that I find the hardest, but often the most valuable, about writing tests, particularly in Javascript where the public vs private is only enforced by convention.

[balmas]

that is a really useful article. thank you! I think it might be a good time for us to decide on which of those we think are the most important to follow for where we are with our development. Let's discuss at our next check-in.

[kirlat]

where the public vs private is only enforced by convention.

Private fields proposal is already implemented in node.js, Chrome and Opera, and will be available in Edge when a Chromium-based version will ship (that is planned on January 15th). Firefox and Safari are under works, but still it might be not in the future too distant.

As we're using _ to designate private members and methods it would be as simple as to replace underscores with # to make them "truly" private. But if it's a private method we're testing now we'll loose that ability with that change because private members and methods are not available outside of the class instance. I think we should keep this in mind while planning and creating our tests.