Add test for task 3 of wizards and warriors exercise #1954
Conversation
There was a problem hiding this comment.
EDIT: Wait... I just realized that we cannot call wizard.Vulnerable() at this point because the student has not defined that method yet! How else could we test this? spellPrepared is a private variable.
I don't think this is true, as task 2 has the student implement the Vulnerable() method on the base Character class which means that it also can be called on the Wizard.
Moreover, once the wizard has prepared a spell, we can no longer test whether they are vulnerable by default!
True, but the idea is that the wizard has custom behavior that overrides the default behavior.
Moreover, once the wizard has prepared a spell, we can no longer test whether they are vulnerable by default! Perhaps we could make spellPrepared public?
The idea is that this information is hidden, to have students become familiar with encapsulation.
just thought it was confusing that there was no test for that particular task.
Yeah, this is a result of the PrepareSpell method only having a side-effect and not returning any value. We chose to have a separate task as we felt that it helped focus the student on just implementing that one feature. An alternative would be to combine task 3 and 4, but we usually want to have the students only implement one method at a time.
p.s. the Wizard_is_vulnerable test is a duplicate of Wizard_with_no_prepared_spell_is_vulnerable and the former should be removed
| @@ -45,6 +45,8 @@ Implement the `Wizard.PrepareSpell()` method to allow a Wizard to prepare a spel | |||
| ```csharp | |||
| var wizard = new Wizard(); | |||
| wizard.PrepareSpell(); | |||
| wizard.Vulnerable(); | |||
There was a problem hiding this comment.
I don't think that the vulnerability of Wizards should be a concern at that point.
Since the next task of the exercise is setting the wizard to be not vulnerable after preparing a spell, at this point they should only be concerned with having a boolean property "IsSpellPrepared" set to true;
| @@ -27,6 +27,15 @@ public void Warrior_is_not_vulnerable() | |||
| Assert.False(warrior.Vulnerable()); | |||
| } | |||
|
|
|||
| [Fact] | |||
| [Task(3)] | |||
| public void Wizard_is_not_vulnerable_after_preparing_spell() | |||
There was a problem hiding this comment.
This is the exact same test as the one starting on line 49, and I don't think it concerns Task 3, since up to that point, it doesn't matter the vulnerability of the wizard after preparing a spell.
It might be useful to add another method called IsSpellPrepared() that you could use to test if the state of the spell is actually being saved.
Issue: #1953
I added a unit test for task 3, and updated the task instructions to show the expected output.
I'm a bit out of my element here, so I will totally understand if this is partially or completely wrong/unnecessary. I just thought it was confusing that there was no test for that particular task. It still seems slightly weird though, because the following task also states that the wizard should not be vulnerable after preparing a spell. Perhaps this could be made clearer somehow.
EDIT: Wait... I just realized that we cannot call
wizard.Vulnerable()at this point because the student has not defined that method yet! How else could we test this?spellPreparedis a private variable.Moreover, once the wizard has prepared a spell, we can no longer test whether they are vulnerable by default! Perhaps we could make
spellPreparedpublic?