Aurora Review

[dhalverson]

I think I'm doing this right.

[aziobrow]

👍

[aziobrow]

👍

[aziobrow]

this could use a test for the name and category, since those are methods (as per the interaction pattern, and because they are attributes) that a dish object should respond to. This is how I would write a test like that:

def test_it_has_attributes
   dish = Dish.new("Couscous Salad", :appetizer)
    assert_equal "Couscous Salad", dish.name 
    assert_equal :appetizer, dish.category 
end

[aziobrow]

when we start off tests with it, it conventionally refers to the object that you're testing-- in this case, an instance of potluck. So when you say it is an empty array, it sounds like you're saying that a potluck object should be an empty array, which is inaccurate. So a better name for this test would be something like test_it_has_dishes, which is more general but completely adequate, or test_it_starts_with_no_dishes if you want to be more descriptive.

[aziobrow]

nice job testing the attributes here (date and dishes)

[aziobrow]

totally fine to break these tests apart like you did, or to do it like i showed you above where you roll them into one test_it_has_attributes and include multiple assertions. whatever makes more sense to you.

[aziobrow]

this test shouldn't be here-- this is the potluck test, and you're testing dishes. You've already tested in your dish test that a dish can exist, so this test is redundant and would be confusing to another developer looking at this code.

[aziobrow]

same for this one.

[aziobrow]

this is great! love that you started off by asserting that there were no dishes in line 42, and then added a dish, and then asserted that it is now an array with couscous salad (a dish instance) inside. good progression testing. you might even consider adding a second one:

 def test_it_can_add_dish
    couscous_salad = Dish.new("Couscous Salad", :appetizer)
     fruit_salad = Dish.new("Fruit Salad", :appetizer)

    potluck = Potluck.new("7-13-18")

    assert_equal [], potluck.dishes
    potluck.add_dish(couscous_salad)

    assert_equal [couscous_salad], potluck.dishes

    potluck.add_dish(fruit_salad)

     assert_equal [couscous_salad, fruit_salad], potluck.dishes
end 

Strictly speaking, adding the second dish isn't necessary, but in general when writing code, you can sometimes get accidentally lucky in writing a method that works one time-- testing a behavior twice with different data ensures that you didn't get accidentally lucky.

[aziobrow]

this name should be more generic-- there's nothing special about adding couscous salad in particular, you can add any dish and it should work.

something like test_it_can_add_dish

[aziobrow]

this test is exactly the same as the last one in terms of what it's doing and isn't testing anything new-- you'd be better off doing something like what i showed above, where you add two dishes in a single test to make sure a potluck can have more than one dish if you add them multiple times. if a test isn't testing any new behavior, you can just leave it out.

[aziobrow]

ooh! haha, i'm reading these tests one at a time and now that I've gotten to the bottom, you did almost exactly what i recommended above, which is great. So this is a really good thought; testing length though is not a good test, because just because something is the right length doesn't mean that it's a collection of the right stuff (if potluck.dishes was an array of strings, this test would still pass, even though it should actually be an array of dish objects), so it can give you false positives.

So a better assertion would be similar to what I suggested above: assert_equal [couscous_salad, cocktail_meatballs], potluck.dishes

That assertion covers that it should have a length of 2 (because you are specifically asserting two things in the array), but also that they are specifically the couscous_salad dish object and the cocktail_meatballs dish object, not just two of something random.

[aziobrow]

So all told, based on my feedback to you, my entire potluck test class would look something like this:

class PotluckTest < Minitest::Test

  def test_it_exists
    potluck = Potluck.new("7-13-18")

    assert_instance_of Potluck, potluck
  end

  def test_it_has_attributes
    potluck = Potluck.new("7-13-18")

    assert_equal "7-13-18", potluck.date
    assert_equal [], potluck.dishes
  end


   def test_it_can_add_dishes
    couscous_salad = Dish.new("Couscous Salad", :appetizer)
    cocktail_meatballs = Dish.new("Cocktail Meatballs", :entre)
    potluck = Potluck.new("7-13-18")

    assert_equal [], potluck.dishes
    potluck.add_dish(couscous_salad)
    potluck.add_dish(cocktail_meatballs)

    assert_equal [couscous_salad, cocktail_meatballs], potluck.dishes
  end
end 

[aziobrow]

this goes along with what we talked about yesterday; more or less, one test per method. The exception (and this is personal preference) is that I like to put all my attributes' starting values into one test, and since date and dishes are both attributes, i put the assertion for both into one test. You don't have to do that by any means-- it's totally find to do as you did and write a separate test for date and another one for dishes as an empty array, but the above is how i'd do it.

potluck.new
potluck instance variables (potluck.date/potluck.dishes)
potluck.add_dish

[aziobrow]

Overall, you definitely have all the right ideas! Nice work.

The big things to think about for now are:

1. you want to completely and thoroughly test (and you're making a clear effort at that) but you don't want to redundantly test. So testing Dish stuff in Potluck test is redundant, as is writing two separate tests for adding a single dish, so the extras can just be cut out.

2. naming is important because it allows someone at a glance to read the name of your test and know what it's testing. a good rule of thumb is to always include the name of the method that you're testing in the name (except for test_it_exists and attributes, if you decide to test all attributes in one test the way that I described in a comment). test_it_has_a_date, test_it_can_add_dish, etc (date and add_dish are specific method names for potluck)

You're completely on the right track! Just keep an eye out of those couple things and you're going to be great. Always happy to review more code of any sort when it's helpful.