When it comes to changes to the Database, we have a tool (which I will call DBPatcher) which runs your changes, runs the Unit Tests and runs Code Analysis (finds bad formatting, violations of coding standards, common mistakes etc). So it is vital that this passes successfully before you send your code to review.
I was doing a Code Review aka Pull Request (PR) and I saw evidence that they hadn’t run it through this DBPatcher tool.
Ronald was eager to get his code checked in, so wanted me to approve. However, I wanted them to run the tool and fix any issues first. The conversation went like this:
[Thursday 8:21 AM] Ronald
Can we complete the PR? do you have any doubts on it
[Thursday 8:23 AM] Me
I'm convinced DBPatcher will flag those select statements because there is a mix of tabs and spaces
<yes it is trivial to flag, but DBPatcher will flag this, so this is evidence they haven’t run it. There could be other errors too, but I will let DBPatcher find them>
[Thursday 8:23 AM] Ronald
OK, thank you. I will complete the PR
[Thursday 8:25 AM] Me
what? I am saying the DB patcher will give you errors
[Thursday 8:26 AM] Ronald
sorry for misunderstanding
I ran it in the morning. We didn't get any error for our DB changes and unit testing also didn't throw any error for our code
<he attempts to send me a screenshot of the final result but it didn’t seem to transfer>
[Thursday 8:44 AM] Me
The image isn't showing for me. But since I started running DBPatcher when you messaged me, and mine has just finished, I can only assume you disabled the "Run Code Analysis" to speed it up
[Thursday 8:45 AM] Me
In fact, there's some failing unit tests too
<this is contrary to what Ronald claimed. He said there were no Code Analysis errors and no Unit Test failures, and I see both.
[Thursday 8:45 AM] Ronald
I have enabled those and haven't unchecked it before running the patch
[Thursday 8:45 AM] Me
What is in the output window?
[Thursday 8:46 AM] Ronald
yes there are some errors, but not related to our code and our schema
[Thursday 8:48 AM] Me
DataWarehouse
Error on line: 12
ColumnListFormatting: Select column list incorrectly formatted
<clearly his code>
[Thursday 8:50 AM] Ronald
oh ok
[Thursday 1:19 PM] Ronald
we resolved formatting in our SQL commands
we couldn't find which unit testing is failing and we are not sure if this unit test is part of our project. Can you help us with this one ?
[Thursday 1:21 PM] Me
|20|[DataWarehouseTest].[Test1] |Error |
|21|[DataWarehouseTest].[Test2] |Error |
|22|[DataWarehouseTest].[Test3] |Error |
|23|[DataWarehouseTest].[Test4] |Error |
|24|[DataWarehouseTest].[Test5] |Error |
[Thursday 1:26 PM] Ronald
I ran the DB patcher 20mins ago with the code analysis checked and we checked the output results also, we couldn't find anything related to DataWarehouseTest
Attached the DB patcher output result we got
[DBPatcher OutputResult.txt]
<I look at the file. It has hundreds of errors, so it is hard to make sense of. His database is clearly screwed. No wonder it was running quick and he couldn’t see any Unit Test errors; they simply weren’t running>
[Thursday 1:31 PM] Me
your database looks absolutely messed up. You shouldn't have those errors. The unit tests are failing to run
C:\DatabasePatcher\tSQLt\RunAllUnitTests.sql
Could not find stored procedure 'tSQLt.RunAll'.
you need a new database.
[Thursday 5:50 PM] Ronald
Thanks for notifying us of these issues.
Now we have fixed these issues and ran the patch, and there were no issues with our project.
[Thursday 5:51 PM] Ronald
please review it from your side
I then look through their changes which fixed the unit test. With Unit Tests, you usually create a variable called “Expected” then set that manually. Then you create an “Actual” variable and this is set based on the actual code. They had those statements as normal, but then they had added this:
update #ActualResult set SessionGuid = '38090f0d-3496-48c3-a991-a0220fe3b58f', SlotGuid = '0b817794-7ffb-4ae3-8013-a7847a1b2139';
So this means their code isn’t returning the correct result, but they are then manipulating the result (#ActualResult) to force it to match – so the test passes. They could have just changed the Expected result, but that would be sabotage anyway. Why would they knowingly break a feature like this?
Anyone who is serious about software development shouldn’t be doing this. They have “Senior” in their job title, and this change was approved by two of their team members. It was up to me to be the gatekeeper and reject this change.
[3:51 PM] Ronald
Sorry for the unit test update statement, I have removed those and all the unit tests are passing correctly.
Sorry, that was some typo.
A typo!? How can you possibly claim that was a typo? “Sorry, I accidentally bashed on the keyboard and somehow produced a sequence of characters that was valid: not only to be executed without error, but for the unit tests to pass too.”
I also don’t understand how you can have hundreds of errors and just continue working like everything is fine. Then when someone is telling you something is wrong, you still pretend everything is fine. When I tell him he hasn’t run DBPatcher, why didn’t he respond with “I did, but there were loads of errors. Can you help me fix this?” Proceeding like he did just wasted my time, created unnecessary friction and made himself look like a complete idiot.
Stranger Coding Tings 