When I was still doing consulting, I had a client that wanted to create One App To Rule Them All: all of their business functions (and they had many) available in one single Angular application. They hoped each business unit would have their own module, but the whole thing could be tied together into one coherent experience by setting global stylesheets.

I am a professional, so I muted myself before I started laughing at them. I did give them some guidance, but also tried to set expectations. Ignore the technical challenges. The political challenges of getting every software team in the organization, the contracting teams they would bring in, the management teams that needed direction, all headed in the same direction were likely insurmountable.

Brian isn’t in the same situation, but Brian has been receiving code from a team of contractors from Initech. The Initech contractors have been a problem from the very start of the project. Specifically, they are contractors, and very expensive ones. They know that they are very expensive, and thus have concluded that they must also be very smart. Smarter than Brian and his peers.

So, when Brian does a code review and finds their code doesn’t even approach his company’s basic standards for code quality, they ignore him. When he points out that they’ve created serious performance problems by refusing to follow his organization’s best practices, they ignore him and bill a few extra hours that week. When the project timeline slips, and he starts asking about their methodology, they refuse to tell him a single thing about how they work beyond, “We’re Agile.”

To the shock of the contractors and the management paying the bills, sprint demos started to fail. QA dashboards went red. Implementation of key features got pushed back farther and farther. In response, management decided to give Brian more supervisory responsibility over the contractors, starting with a thorough code review.

He’s been reviewing the code in detail, and has this to say:

Phrases like ‘depressingly awful’ are likely to feature in my final report (the review is still in progress) but this little gem from testing jumped out at me.

  it('should detect change', () => {
    fixture.detectChanges();
    const dt: OcTableComponent = fixture.componentInstance.dt;
    expect(dt).toEqual(fixture.componentInstance.dt);
  });

This is a Jasmine unit test, which takes a behavioral approach to testing. The it method expects a string describing what we expect “it” to do (“it”, in this context, being one unit of a larger feature), and a callback function which implements the actual test.

Right at the start, it('should detect change',…) reeks of a bad unit test. Doubly so when we see what changes they’re detecting: fixture.detectChanges()

Angular, when running in a browser context, automatically syncs the DOM elements it manages with the underlying model. You can’t do that in a unit test, because there isn’t an actual DOM to interact with, so Angular’s unit test framework allows you to trigger that by calling detectChanges.

Essentially, you invoke this any time you do something that’s supposed to impact the UI state from a unit test, so that you can accurately make assertions about the UI state at that point. What you don’t do is just, y’know, invoke it for no reason. It doesn’t hurt anything, it’s just not useful.

But it’s the meat of the test where things really go awry.

We set the variable dt to be equal to fixture.componentInstance.dt. Then we assert that dt is equal to fixture.componentInstance.dt. Which it clearly is, because we just set it.

The test is named “should detect changes”, which gives us the sense that they were attempting to unit test the Angular test fixture’s detectChanges method. That’s worse than writing unit tests for built-in methods, it’s writing a unit test for a vendor-supplied test fixture: testing the thing that helps you test.

But then we don’t change anything. In the end, this unit test simply asserts that the assignment operator works as expected. So it’s also worse than a test for a built-in method, it’s a test for a basic language feature.

This unit test manages, in a few compact lines, to not simply be bad, but is “not even wrong”. This is the kind of code which populates the entire code base. As Brian writes:

I still have about half this review to go and I dread to think what other errors I may find.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.