Skip to content
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

Suggested reorder of tests for space-age #302

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmyShackles
Copy link

Observations:

  • The instructions don't say anything about rounding but the tests expect rounded answers
  • The algorithm necessary for calculating age on Earth is slightly different than that to calculate age on any other planet
  • There is already a test for checking that the value of age on Earth is accurate
  • Each 'it' block testing for the correct age on different planets begins with an 'expect' for Earth age
  • When there are multiple expects in an 'it' block and the first expect fails, only that expect is shown as failing when running the test

In the event that the programmer has not implemented the necessary rounding, it is possible for them to be led to believe that they simply have a rounding error when it's possible that their algorithm for determining the age on other planets is fundamentally flawed.

Suggestion:
It might be beneficial to have the order of the expects flipped so that each planet test first tests the target planet and then additionally checks for the accurate Earth age.

@pacman-bot
Copy link

Danger has errored

[!] Invalid Dangerfile file: undefined local variable or method shortFile for #<Danger::Dangerfile:0x00000000039b9148>

 #  from Dangerfile:46
 #  -------------------------------------------
 #        # get away from doing inline comments since they are buggy as of Sep-2016
 >        shortFile.prepend("/") unless source_file[0] == '/'
 #  
 #  -------------------------------------------

Generated by 🚫 Danger

@SleeplessByte
Copy link
Member

It might be beneficial to have the order of the expects flipped so that each planet test first tests the target planet and then additionally checks for the accurate Earth age.

I really like this. Sorry this took forever to look into -- I'll see if this can be merged as is, or needs to be updated;

Base automatically changed from master to main January 28, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants