Mystery Guest Development Edition
Mystery Guest is the name of an anti-pattern that often appear in tests. In short, it is caused by non explicitly declaring or naming a value, which is tested over that faulty declaration. The issue is not apparent at first, as the test is created at the same time as the fixture or factory that supports it. Eventually, when a change is needed in that support object to test something else, the change unintentionally breaks the first test.
For a test which verifies that the full_name of a user is the combination of
first_name and last_name, those two properties need to appear explicitly on
the test setup. Even if you have user factory, you are not testing the
factory, but your own user with first_name and last_name. If the users
in your application also need, say, an address not being tested in
this example, the factory can provide that.
So the question arises: are tests the only ones affected by this anti-pattern?
Let’s see an example.
In our application, we had a CheckIn model and a associated Progress
to record when the each step of a check-in has been completed. We
want to calculate, among other things, the duration of each check-in for
reporting purposes. We had something like this:
1
2
3
4
5
6
7
8
# models/check_in.rb
class CheckIn < ApplicationRecord
has_one :progress, dependent: :destroy
before_create -> { self.progress ||= Progress.new }
...
end
1
2
3
4
5
6
7
8
9
10
11
12
# models/progress.rb
Class Progress < ApplicationRecord
belongs_to :check_in
def duration
if check_in_completed_at.present?
created_at - check_in_completed_at
end
end
...
end
The duration method in Progress is the interesting one. We didn’t need to
use the CheckIn created_at field because a Progress was created at the
same time. So, to simplify and optimize, we used the Progress created_at
method instead. There is no issue whatsoever with this approach. Or is
there?
After some time, we decided to include events happening before the patient arrives to the practice, which is before the check-in.
We created a new model called Engagement. As we still wanted to keep track of
steps completed before and during the check-in, we associated our Progress to
the Engagement which we moved up from the CheckIn.
1
2
3
4
5
6
7
8
9
# models/engagement.rb
class Engagement < ApplicationRecord
has_one :check_in, dependent: :destroy
has_one :progress, dependent: :destroy
before_create -> { self.progress ||= Progress.new }
...
end
1
2
3
4
5
6
# models/check_in.rb
class CheckIn < ApplicationRecord
belongs_to :engagement
...
end
1
2
3
4
5
6
7
8
9
10
11
12
# models/progress.rb
class Progress < ApplicationRecord
belongs_to :engagement
def duration
if check_in_completed_at.present?
check_in_completed_at - created_at
end
end
...
end
You have probably already seen the problem. Progresses are now sometimes created
long before the check-in starts. In some cases, days before. And since then,
our calculations of check-in duration started failing. And that is not the
biggest problem. The main issue is how hard this is to detect. Tests are
green, and there’s no clear indication that the calculations are wrong until you
see the actual numbers.
We thought we knew all our dependencies, but a mysterious guest appeared! We
assumed that progresses would always be check_in progresses, but that’s not
true anymore. Old check_in progresses are now engagement ones.
Once again, we trusted the underlying object supporting our assumptions, but it
changed on us.
The fix is arguably even more interesting than the bug. As a first step, we
specify that we want check_in.created_at timestamp and not the progress
one. Also, as a progress now is associated with an Engagement, we can
change the name of the method to specify what we are talking about.
1
2
3
4
5
6
7
8
9
10
11
12
# models/progress.rb
class Progress < ApplicationRecord
belongs_to :engagement
def check_in_duration
if check_in_completed_at.present?
check_in_completed_at - check_in.created_at
end
end
...
end
Now, method name and implementation are telling us something else. All the
references to check_in indicate this method probably suffers from “feature
envy”, and should be in the CheckIn object instead of here. In our way of
fixing an issue we found a better design for our code as well!
This is the final version:
1
2
3
4
5
6
7
8
9
# models/engagement.rb
class Engagement < ApplicationRecord
has_one :check_in, dependent: :destroy
has_one :progress, dependent: :destroy
before_create -> { self.progress ||= Progress.new }
...
end
1
2
3
4
5
6
7
8
9
10
11
12
13
14
# models/check_in.rb
CheckIn < ApplicationRecord
belongs_to :engagement
delegate :completed_timestamp, to: :progress, prefix: true
def duration
if completed_timestamp.present?
progress_completed_timestamp - created_at
end
end
...
end
1
2
3
4
5
6
7
8
9
10
# models/progress.rb
Class Progress < ApplicationRecord
belongs_to :engagement
def check_in_completed_timestamp
check_in_completed_at
end
...
end
In summary, there were two main reasons for a development mystery guest here:
We trusted that objects supporting our code assumptions wouldn’t change on us. And because of that, we didn’t specify the information needed from them. (The same happens during testing when we trust fixtures or factories to have the data for us to pass the test, instead of declaring it during setup).
We optimized too early. It is simpler to call a method in the current object than to ask for the information to some other one. But even if simpler, it may not be correct with future changes in the code.
Avoid assuming that the information depending on other objects will always be as correct as it is today.
Always specify what you need. And forget about early “optimizations”. Do them when they make sense and are needed. That’s how you make sure you prevent mystery guests.