We're continuing our work towards understanding how we can have near 100% test coverage.
I get pretty annoyed at the Activity class sitting there at super low percentages and feeling like there's nothing I should do. Not that I feel there's nothing I can do; just that "it's a boundary layer" ... A lot of the same arguments I made for the Adapter. Welllll.... That's at 100% now.
I've tried a few techniques in the past to make what little functionality I leave in the UI testable. I'll be pulling these approaches in again; with actual tests against them; not the theoretical tests I did before.
I did a little pre-work on this while at the gymnastics studio. Exploring the possibilities w/o TDD driving it. Even now I'm not going to have TDD drive it. Not because I couldn't; but because... I'm lazy? I don't have tests already... Am I going to continue to make excuses? Yes.
Short version - I don't know enough TDD to be comfortable writing a test and then re-writing a HUGE chunk of code to get it to pass. That's supposed to be avoided.
... sigh ...
If the test you've written (or are thinking of writing) requires a large code change - You've got the wrong test.
... I was thinking of a test that required a large code change.
I have a test requires zero code refactor - just new code. This is the test I should be writing.
I have to call out that this stance of a test being "the wrong test" is new for me; and I think it's huge. This concept has been doubly emphasized recently by some posts from Uncle Bob and the Extreme Programming: Applied book.
Tests
We're going to start by writing a test for the constructor.
The reason we're going to start here is that there's a class variable getting set inline
public class TopStoriesActivity extends AppCompatActivity {
private TopStoriesActivityBridge topStoriesActivityBridge = new TopStoriesActivityBridge(this);
...
}
and this makes testing very very hard. I did it because... hangs head ... it's the UI boundary, it doesn't need to be tested.
Look - I lacked a pair; I'm making mistakes!
Anyway; I have no idea why it's not final. I final everything.
The constructor we want to create is going to be part of a "Double Constructor" dependency injection pattern I follow. And Yea... it's (currently) arguably a constructor for test. I don't like it; but I like tests more than I dislike this.
If it's not testable; it's not trustable.
Creating a test class, TopStoriesActivityTests we can create the first test - for functionality we already have.
@Test
public void ctorShouldHaveNoParam(){
new TopStoriesActivity();
}
While I'm not getting a red on this test due to not starting off TDD; it's a required test moving forward.
The test passes... a little surprising since it is likely calling some Support Library stuff...
With that passing; we can move onto the next test; passing in the Bridge so we don't create one at class instantiation.
@Test
public void ctorShouldTakeBridge(){
new TopStoriesActivity(new TopStoriesActivityBridge(new FakeTopStoriesActivity()));
}
We'll use the refactoring suggestions to create the constructor.
public TopStoriesActivity(final TopStoriesActivityBridge topStoriesActivityBridge) {
}
Now we can't compile. There is no longer a zero arg constructor. A zero arg constructor is required for Android; we need to get the test passing again.
public TopStoriesActivity(){
}
That was challenging. :-P
Running the TopStoriesActivity tests - Both pass.
Not really sure how to test these other than - Now it exists. It builds and the test passes... Kinda all I was aiming for.
Using the double constructor
Now that we have the constructor for dependency injection, we need to ensure we actually use the object we're passing in.
Looking at the TopStoriesActivity
public class TopStoriesActivity extends AppCompatActivity {
private final TopStoriesActivityBridge topStoriesActivityBridge = new TopStoriesActivityBridge(this);
private RecyclerView rvTopStories;
public TopStoriesActivity(){}
public TopStoriesActivity(final TopStoriesActivityBridge topStoriesActivityBridge) {}
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.top_stories_activity);
bindViews();
configureViews();
loadData();
}
private void loadData() {
topStoriesActivityBridge.loadData();
}
private void configureViews() {
configureTopStories();
}
private void configureTopStories() {
rvTopStories.setAdapter(topStoriesActivityBridge.createTopStoriesAdapter());
rvTopStories.setLayoutManager(new LinearLayoutManager(this));
rvTopStories.addItemDecoration(new DividerItemDecoration(rvTopStories.getContext(), DividerItemDecoration.VERTICAL));
}
private void bindViews() {
rvTopStories = (RecyclerView)findViewById(R.id.rv_top_stories);
}
/* package */ void notifyTopStoriesChanged(){
rvTopStories.getAdapter().notifyDataSetChanged();
}
/* package */ void notifyTopStoryChanged(final int index){
rvTopStories.getAdapter().notifyItemChanged(index);
}
}
there's currently only one place it's used - in a private method.
Uncle Bob says
Tests trump Encapsulation.
I'm sure this isn't "Do whatever you want just expose it for tests". he continues
That means that tests win. No test can be denied access to a variable simply to maintain encapsulation.
If you can't test it any other way; it's gonna be opened for testing.
I must highlight that the post this is from is about a singleton. Singletons are anti-patterns with very narrow scope of reasonable use. Cracking open the encapsulation in this case is really the only way to do it. Which is part of why the Singleton is a troublesome pattern to use; it complicates testing.
I'll argue against a singleton every time until shown it's the best solution. I find that distributed libraries are about the only place that can consistently win to utilize the singleton.
Now; back to the validating the mock/fake TopStoriesActivityBridge is used. Is that the correct test?
It's going to require some changes.
Let's write out those changes; kinda think it through out loud... I wonder how well the blog can serve as my pair; or rubber duck...
Right now the complication on getting loadData invoked is that it's a private method; I don't want to just open it up. It's called from onCreate which will throw an exception when super.onCreate is called.
While this isn't as "algorithm" as other things; we could transform this into the Template Method pattern.
There's a couple approaches to this; and I've done it before in PoC examples. One example I had a few PoC's in, including the Template Method, can be seen here. Reproduced for ease of reading
public abstract class MainActivity extends BaseActivity {
private TextView simple;
private MainViewModel mainViewModel;
public MainActivity(){
mainViewModel = new MainViewModel(this);
}
@Override
protected void bindViews() {
simple = (TextView)findViewById(R.id.simpleValue);
}
@Override
protected void postOnCreate(Bundle savedInstanceState){
//The layout is part of the view(s)
setContentView(R.layout.activity_main);
}
@Override
protected void postOnResume(){
mainViewModel.render();
}
/* protected */ TextView getSimpleView(){
return simple;
}
}
The methods for the type of behavior is put into method calls; which get invoked by the base class' onCreate.
This is a fairly simple form; I have more complex expectations as they arise; but working through our current example; if we just delegate the whole of onCreate minus the super call... Sounds like a base class.
OR - What I've done before as I've faced opposition to the Template Method implementation; is to create a package-private method of onCreateFunctionality which is invoked by the onCreate. This is less code.
This is the correct change to make. We can refactor into base classes as we find this commonality; but currently; we're at a single Activity (I'm ignoring Main; it'll go away) and we don't have "common" functionality to abstract.
I don't like the test I was about to write. I wrote the name - onCreateShouldInvokeLoadData. Then I shrunk it to onCreateShouldLoadData. Well... yes; it should. But... Do I need to check method invocation? If I refactor out the Bridge; then that test breaks, even though the functionality hasn't changed. I started to do a test of the implementation.
We have the TopStoriesActivity above; if we fake the adapter in the rvTopStories then we can be sure that the adapter is notified of the data change; regardless of how we get the data.
This test becomes tightly coupled to the RecyclerView and it's Adapter. If we're changing that much of the UI; the UI is essentially entirely changing; or I just don't know how to do it better; I can accept that.
Can we just straight mock the RecyclerView? If so we can shove it into the double constructor and go from there.
Let's go with that route.
The onCreateShouldLoadData test now looks like
@Test
public void onCreateShouldLoadData(){
final RecyclerView mockRecyclerView = Mockito.mock(RecyclerView.class);
final RecyclerView.Adapter mockAdapter = Mockito.mock(RecyclerView.Adapter.class);
Mockito.doNothing().when(mockAdapter).notifyDataSetChanged();
Mockito.when(mockRecyclerView.getAdapter()).thenReturn(mockAdapter);
new TopStoriesActivity(new TopStoriesActivityBridge(new FakeTopStoriesActivity()), mockRecyclerView);
Mockito.verify(mockAdapter).notifyDataSetChanged();
}
There's a bit of Obvious Implementation going on; but I'm ok with that.
With this test in place, the check from the earlier double constructor test is being duplicated; once we get a Green Bar; we'll refactor it away.
Our Assert is failing because it's not wired up to call yet.
Looking at how to wire this up; I think I need to re-wire and not use a real TopStoriesActivityBridge. Let's use the Fake version we've set up for other tests.
A little refactoring of the FakeTopStoriesActivityBridge and ... we've forgotten to use the new method we set up...
With how I'm refactoring; I shouldn't have been ok with the "Obvious Implementation" from above - The test is super wrong. I'm working up the correct form now. It'll be a little bit of time for me
...
and none for you!
The method now looks like
@Test
public void onCreateShouldLoadData(){
final RecyclerView mockRecyclerView = Mockito.mock(RecyclerView.class);
final RecyclerView.Adapter mockAdapter = Mockito.mock(RecyclerView.Adapter.class);
Mockito.doNothing().when(mockAdapter).notifyDataSetChanged();
Mockito.when(mockRecyclerView.getAdapter()).thenReturn(mockAdapter);
final FakeTopStoriesActivityBridge fakeTopStoriesActivityBridge = new FakeTopStoriesActivityBridge(new FakeTopStoriesActivity());
final TopStoriesActivity actualTopStoriesActivity = new TopStoriesActivity(fakeTopStoriesActivityBridge, mockRecyclerView);
fakeTopStoriesActivityBridge.setActualTopStoriesActivity(actualTopStoriesActivity);
actualTopStoriesActivity.onCreateFunctionality();
Mockito.verify(mockAdapter).notifyDataSetChanged();
}
and yea... I'm still doing some Obvious Implementation. It's the test I want to pass.
Updating the onCreate to the following form get's the test passing
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.top_stories_activity);
bindViews();
configureViews();
onCreateFunctionality();
}
/* package */ void onCreateFunctionality() {
loadData();
}
Looking at the code a little more; while that was a great process to go through; The notifyTopStor[ies|y]Changed methods could have been tested first. Let's implement some quick tests for those as they are simple.
@Test
public void notifyTopStoriesChangedShouldUpdateAdapter(){
final RecyclerView mockRecyclerView = Mockito.mock(RecyclerView.class);
final RecyclerView.Adapter mockAdapter = Mockito.mock(RecyclerView.Adapter.class);
Mockito.doNothing().when(mockAdapter).notifyDataSetChanged();
Mockito.when(mockRecyclerView.getAdapter()).thenReturn(mockAdapter);
new TopStoriesActivity(new FakeTopStoriesActivityBridge(
new FakeTopStoriesActivity()), mockRecyclerView).notifyTopStoriesChanged();
Mockito.verify(mockAdapter).notifyDataSetChanged();
}
@Test
public void notifyTopStoryChangedShouldUpdateAdapter(){
final int index = RandomValues.nextInt(1000);
final RecyclerView mockRecyclerView = Mockito.mock(RecyclerView.class);
final RecyclerView.Adapter mockAdapter = Mockito.mock(RecyclerView.Adapter.class);
Mockito.doNothing().when(mockAdapter).notifyDataSetChanged();
Mockito.when(mockRecyclerView.getAdapter()).thenReturn(mockAdapter);
new TopStoriesActivity(new FakeTopStoriesActivityBridge(
new FakeTopStoriesActivity()), mockRecyclerView).notifyTopStoryChanged(index);
Mockito.verify(mockAdapter).notifyItemChanged(index);
}
I've got some commonality happening here; let's update to use the @Mock annotation.
Now the tests are nice and succinct
@Test
public void notifyTopStoriesChangedShouldUpdateAdapter(){
new TopStoriesActivity(null, mockRecyclerView).notifyTopStoriesChanged();
Mockito.verify(mockAdapter).notifyDataSetChanged();
}
@Test
public void notifyTopStoryChangedShouldUpdateAdapter(){
final int index = RandomValues.nextInt(1000);
new TopStoriesActivity(null, mockRecyclerView).notifyTopStoryChanged(index);
Mockito.verify(mockAdapter).notifyItemChanged(index);
}
Configuring Widgets
This next adaption I don't think will apply many places, yet. Early days; still gotta work things out.
It's configuring the controls.
As my onCreate is structured; I'll "find" all the controls, set them; then configure them.
This helps keep the scope and functionality of each function narrowed.
I'm not sure how to test the bindViews method right now; so I'll aim at the configureViews. Yes it just chains, so I can get that easy. rolls eyes
What I need to do is prepare to configure the rvTopStories object.
Huh... if I keep piling things into the onCreateFunctionality method; then I'll have to prepare the mocks to support loadData and every other method just to test one of them. While this'll just be a method for each; I don't want to have to mock that much. I'm cracking the encapsulation.
It's this or a base class; which cracks encapsulation anyway.
This feels very wrong; I'm just not sure what else to try... Which I hate. I don't like coding things that feel wrong.
Well... I'm configuring a RecyclerView to be a TopStoriesRecyclerView... Why not create that?
I'm not looking to create a new widget (not at the moment) but to encapsulate the functionality.
TopStoriesRecyclerView
Guess I need to TDD this recycler view.
A few dozen lines of test later; we have a TDD'd TopStoriesRecyclerView
/* package */ class TopStoriesRecyclerView{
private final RecyclerView recyclerView;
private final RecyclerView.Adapter adapter;
/* package */ TopStoriesRecyclerView(final RecyclerView recyclerView, RecyclerView.Adapter adapter) {
if(recyclerView == null) { throw new IllegalArgumentException("recyclerView can not be null"); }
if(adapter == null) { throw new IllegalArgumentException("adapter can not be null"); }
this.recyclerView = recyclerView;
this.adapter = adapter;
configureRecyclerView();
}
private void configureRecyclerView() {
recyclerView.setAdapter(adapter);
recyclerView.setLayoutManager(new LinearLayoutManager(recyclerView.getContext()));
recyclerView.addItemDecoration(new DividerItemDecoration(recyclerView.getContext(), DividerItemDecoration.VERTICAL));
}
/* package */ void notifyTopStoriesChanged() {
adapter.notifyDataSetChanged();
}
/* package */ void notifyTopStoryChanged(final int position) {
adapter.notifyItemChanged(position);
}
}
via TopStoriesRecyclerViewTests.
public class TopStoriesRecyclerViewTests extends QacTestClass{
@Mock RecyclerView mockRecyclerView;
@Mock RecyclerView.Adapter mockAdapter;
@Mock Context mockContext;
@Mock TypedArray mockTypedArray;
@Before
public void setup(){
Mockito.when(mockRecyclerView.getContext()).thenReturn(mockContext);
Mockito.when(mockContext.obtainStyledAttributes(any())).thenReturn(mockTypedArray);
}
@Test
public void ctorShouldThrowIllegalArgumentGivenNullRecyclerView(){
assertThatThrownBy(() -> new TopStoriesRecyclerView(null, mockAdapter))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("recyclerView");
}
@Test
public void ctorShouldThrowIllegalArgumentGivenNullAdapter(){
assertThatThrownBy(() -> new TopStoriesRecyclerView(mockRecyclerView, null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("adapter");
}
@Test
public void ctorShouldTakeRecyclerViewAndAdapter(){
new TopStoriesRecyclerView(mockRecyclerView, mockAdapter);
}
@Test
public void ctorShouldSetRecyclerViewAdapter(){
new TopStoriesRecyclerView(mockRecyclerView, mockAdapter);
Mockito.verify(mockRecyclerView).setAdapter(mockAdapter);
}
@Test
public void ctorShouldSetLayoutManager(){
new TopStoriesRecyclerView(mockRecyclerView, mockAdapter);
Mockito.verify(mockRecyclerView).setLayoutManager(any());
}
@Test
public void ctorShouldSetItemDecoration(){
new TopStoriesRecyclerView(mockRecyclerView, mockAdapter);
Mockito.verify(mockRecyclerView).addItemDecoration(any());
}
@Test
public void notifyTopStoriesChanged(){
new TopStoriesRecyclerView(mockRecyclerView, mockAdapter).notifyTopStoriesChanged();
Mockito.verify(mockAdapter).notifyDataSetChanged();
}
@Test
public void notifyTopStoryChanged(){
final int position = RandomValues.nextInt(1000);
new TopStoriesRecyclerView(mockRecyclerView, mockAdapter).notifyTopStoryChanged(position);
Mockito.verify(mockAdapter).notifyItemChanged(position);
}
}
Now we can extract the RecyclerView configuration from TopStoriesActivity.
Which means we'll need to have open our TopStoriesActivityTests to update. ... Though; this transformation shouldn't affect the behavior. I hope there's no tests to modify.
I've run all tests; they all pass. I'll extract slowly and verify tests at each step.
Adding a class variable for the TopStoriesRecyclerView.
private TopStoriesRecyclerView topStoriesRecyclerView;
replacing the rvTopStories.
private void bindViews() {
rvTopStories = (RecyclerView)findViewById(R.id.rv_top_stories);
}
to
private void bindViews() {
topStoriesRecyclerView = new TopStoriesRecyclerView(
(RecyclerView)findViewById(R.id.rv_top_stories),
topStoriesActivityBridge.createTopStoriesAdapter());
}
Which has broken a couple tests. topStoriesRecyclerView wasn't getting set in the double constructor.
Adding this.topStoriesRecyclerView = new TopStoriesRecyclerView(rvTopStories, topStoriesActivityBridge.createTopStoriesAdapter()); makes it so.
Instead of relying on other things to construct an actual TopStoriesRecyclerView we'll make the quick modification to the constructor to take it in as an arg; then it can be faked how we want it without caring about it's implementation. Ya know - The correct way.
There has been a bit of a refactor to get all the tests cooperating. This has brought up TopStoriesActivity to 87/87. The only hold out is the onCreate method which we can't test due to the super.onCreate call.
I'm not OK with that. It's easy enough to slip in some code and not get it tested. I'm implementing the Template Method pattern for a base activity.
public abstract class QacActivity extends AppCompatActivity {
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
onCreateFunctionality();
}
protected abstract void onCreateFunctionality();
}
This is definitely not my ideal form of this base class; but it's what's required right now.
100%/100%
Running the code coverage we are now at 100%/100% for our activity. While THIS activity wasn't TDD'd; that we can achieve 100% coverage shows that it can be TDD'd completely.
Looking at the package level test results; I've got only 90% of the classes covered... but every class is at 100%/100%... Wha?!?!?
Some additional digging shows that I don't have any tests around the DataLoadStrategy class. Guess what's getting remedied. :)
The code had coverage from the TopStoriesActivityMediator but no direct tests.
Getting that in place doesn't change anything... Oh well.
Low code coverage is a smell. High code coverage is just the absence of a smell; it doesn't say anything positive about the code.
Summary
I've gone through a process to demonstrate that it's possible to configure an Activity to be TDDable.
I still advocate no logic in the UI. Sometimes having the tools to do something bleeds into people doing what they shouldn't. I can't always stop that. I'll advocate that the Activity should be devoid of logic; and leave it at that.
I can TDD an activity; I'm happy with that.