Skip to content
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

Rewrote todos app(examples/todos) to maximize unit testability. #1458

Closed
weicheng113 opened this issue Feb 28, 2016 · 10 comments
Closed

Rewrote todos app(examples/todos) to maximize unit testability. #1458

weicheng113 opened this issue Feb 28, 2016 · 10 comments

Comments

@weicheng113
Copy link

Hi Guys,

I rewrote todos app. I think testability is quite important. You can find it here. The idea behind it is as follows. What do you think?

Purpose

Rewrote todos app(examples/todos) to maximize unit testability.

Theory and Reasoning

Components creation and wiring should happen right in the factories and application entry point, which is one of the keys to make components easy to be unit tested.

This is a practice in OOP world, which was mentioned Nat Pryce(author of Growing Object Oriented Software, Guided by Tests), Misko Hevery(Creator of AngularJS) and other places.

The process of components creation is like an inverted tree, using small components to assemble large ones, started from leaf components. There are two kinds of components, finite instance components and infinite instance components. And infinite ones will be created by factories.

Todos as an Example(examples/todos)

src/components/App.js is the entry point of app. We create and wire up components here.

The components, AddTodo(1 instance), TodoList(1 instance), Footer(1 instance) as well as Link(3 instances), are examples of finite instance components. Whereas, the component, Todo, is an example of infinite instance component, which will be created by component factory, TodoFactory in this case.

As components only ask for minimal direct dependencies at construction, their logic can be easily unit tested, regardless of whether it is a leaf component(e.g., Link) or a branch component(e.g., TodoList).

Unit tests(or specs) for components are under test/components.

Cheers, Cheng

@gaearon
Copy link
Contributor

gaearon commented Feb 28, 2016

Isn’t solving that the point of shallow rendering? You can test that parent components render correctly without relying on the details about their children.

@weicheng113
Copy link
Author

@gaearon , No, this is a general idea for writing a testable app, regardless tools and languages. I find some related links below.
a. Nat Pryce's 4th reply: '...it's the responsibility of the entry point to create and/or obtain the
application's objects and compose them together...'
b. In a Robert C. Martin article, J. B. Rainsberger's reply: '...At the entry point, we make as many of those decisions as possible, creating a graph of loosely-coupled objects...' and Mark Seemann's reply '...wire up the desired object graph in the application’s entry point ... I call this place the Composition Root...'.

@wdhorton
Copy link

Could you submit this as a PR so we could more easily review the changes?

@weicheng113
Copy link
Author

@wdhorton, I have made the PR #1463. There are some conceptual changes. Please review the changes in src/components and test/components, ignoring other parts for now.

@weicheng113
Copy link
Author

@gaearon What do you think about the rewrote of src/components and test/components parts? I believe making app easier to test is pretty important?

@gaearon
Copy link
Contributor

gaearon commented Mar 5, 2016

I described my position in #1463 (comment). To be quite honest, I don’t see what this indirection buys compared to React shallow rendering. I’ll close the issue because this is not something we plan to incorporate into the official examples but I’m happy to keep discussing your approach in this issue, and hear more feedback about what exactly you found problematic to test with the existing approach we currently use.

@gaearon gaearon closed this as completed Mar 5, 2016
@weicheng113
Copy link
Author

@gaearon, Good to hear your comment. I think you did not grab the main point i made here, which could be the unrelated parts i have included in the source code(The PR i made was intended to be incorporated and that is why i said to read src/components and test/components parts only). I have removed all the unrelated things but do not have enough time to explain in detail today. Please hold on #1397. I will find some time tomorrow to explain with the new example code.

Cheers, Cheng

@cwei-bgl
Copy link

cwei-bgl commented Mar 7, 2016

@gaearon , I have made a new PR #1488, in which I have removed all unrelated parts. The topic I focused on here was to show a potential better way to make an app more unit testable, which is a known way in OOP world. The approach, which was already explained in the beginning, is a general idea, which can be applied regardless of the use of classes or functions.

Now that we have a comparable PR(#1471), let me try to point out the key different between them below:
a. Component creation is top down tree structure. Child components are created by parents where required in original approach.

Whereas, in the approach I propose the component creation is aninverted tree. There are only two places where component creation happens, which are application entry and factories. Application entry consists of three top level containers(they have to stay at very top) and their role is composing or assembling the app. And a function, displayTodo, plays factory role for infinite instance components, regardless its form, a function or a class.

b. #1471 has to depend on shallow rendering to accomplish kind of unit testing. The other approach does not.

Let me try to point out key consequences and observations with the two approach:
a. Unit test, or a better name Focused test, is an isolated test against a component or object under test. It is to test whether a component behaves correctly assuming all its collaborators are correct.

In the original top-down approach, as components are created directly in place, there is no way to mock out. In other words, it mixes component creation logic and business/ui logic.

b.The need of a special tool shallow rendering for testing may be a hint that components are not written in a test-friendly way.Take TodoList.spec.js for example, it will become an integration test without shallow rendering, as it contains children or collaborator code.
Depending on how well shallow rendering is, such tests may have more reasons, than it should be, to fail.

Having said that, I feel this may be better to post to react, which I will do.

Cheers, Cheng

@kurtharriger
Copy link

I think @gaearon's opinion was that with shallow rendering unit testing this is a solved issue.

I do however agree that the need for these testing tools in some circles implies insuficient abstration. This is not however a universal truth and many developers particularly functional developers feel that OO goes overboard with abstraction introducing unnecessary abstractions provider factory singleton anyone. Sometimes monkey patching in unit tests is better than introducing complexity everywhere else.

However this does serve to decouple the list item event handling from the list container and perhaps a more compelling reason to illustate todo factory function pattern is issue 822. #822

@weicheng113
Copy link
Author

@kurtharriger Good to hear your comment on the discussion. Regarding the source code complexity, have you compared the modified code under directories component and containers with the current source code? And as for test code complexity, have you compare #1488 with #1471 and #1493?

Functional programming is a general concept regardless of OO or not OO. Scala intends to adopt Functional programming as preferred programming style.

Cheers, Cheng

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

No branches or pull requests

5 participants