-
Notifications
You must be signed in to change notification settings - Fork 885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/unicorn fix #93
base: main
Are you sure you want to change the base?
Conversation
…nd making this an earlier suite
canFly: true | ||
} | ||
|
||
assert.deepEqual(newSamplePegasus, createYourPegasus(newSamplePegasusName, newSamplePegasusColor, newSamplePegasusPattern)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch the order of the args in the parentheses here. You want the fn invokation as the first arg and the expected outcome as the second.
var trainMaryAgain = runSomeMiles(trainMary, 5.2) | ||
assert.equal(registerForRace(trainMaryAgain), 'Congrats Mary, you are now registered for the marathon!') | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test suite looks great!
home: home || 'Beyond the Wall', | ||
size: size || 'Massive', | ||
starksToProtect: [], | ||
protect: function(stark) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that having methods in objects like this is too much OOP. I'm curious what others think! If no one else reviews this, maybe post in the #frontend-curriculum-updates channel with a code snippet and ask what they think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, so far I haven't seen any of the assessments have examples with objects having methods. Potentially this could get moved out into a function that takes a stark in as an argument and returns either a new object or updates its values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall Jeremiah! I think the Ogre and Direwolf tests could be refactored a bit more so they aren't using methods, similar to what Kayla mentioned above. Otherwise, I love the new runner tests and pegasus test! I'm curious, are you still working on writing the road-test.js
file or is that left over?
}; | ||
}; | ||
|
||
function runSomeMiles(runner, miles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the tests for runSomeMiles
! This is a great situation of adding properties to an object if it doesn't exist, or updating it if it does. Cheers!
}); | ||
|
||
it('should not be able to register if they havent trained enough', function() { | ||
//"enough" is the same as the length of a marathon --> 26.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the comment here. I know some of our past assessments had comments with extra context that they had to pay attention to, so this is great practice for them! Nice work on this test suite!
home: home || 'Beyond the Wall', | ||
size: size || 'Massive', | ||
starksToProtect: [], | ||
protect: function(stark) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, so far I haven't seen any of the assessments have examples with objects having methods. Potentially this could get moved out into a function that takes a stark in as an argument and returns either a new object or updates its values?
const stark = new Stark({name: 'John', area: 'King\'s Landing'}); | ||
it('should only be able to protect a Stark if direwolf and Stark locations match', function() { | ||
const direwolf = createMyDirewolf('Ghost'); | ||
const stark = buildAStark({name: 'John', area: 'King\'s Landing'}); | ||
|
||
direwolf.protect(stark); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example here, rather than using a method, potentially we could have a protect
function that takes in an object. And returns an object that has an array of "protected starks".
const human = new Human('Jane'); | ||
it('', function() { | ||
const ogre = describeOgre({name: 'Brak'}); | ||
const human = createHuman('Jane'); | ||
|
||
assert.equal(human.noticesOgre(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a bit tricky since this deals with having to track "state". IE how many times a human has noticed the ogre. I wonder how we could create a function that takes in an ogre object and always returns the same value if given the same input: true or false? Potentially the ogre object has a property of how many times he's been noticed?
|
||
var greeting = "Hi" | ||
|
||
assert.equal(makeAnEntrance(greeting, newSamplePegasus), "Hi! My name is Who Do You Think You Are? I Am!, and I am a safety orange, cape coat Pegasus!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this spicy test! It really will push students to take it one step at a time and compare their solution to the error message. Nice work Jeremiah!
updated:
created:
-pegasus-test