Mystery Guest Development Edition

Sebastian Armano May 10, 2021

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.