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

Suite is missing a test for a pomodoro clock edge case #755

Closed
abdusabri opened this issue Jan 5, 2019 · 7 comments · Fixed by #772
Closed

Suite is missing a test for a pomodoro clock edge case #755

abdusabri opened this issue Jan 5, 2019 · 7 comments · Fixed by #772

Comments

@abdusabri
Copy link
Contributor

@abdusabri abdusabri commented Jan 5, 2019

Issue Description

There is an edge case for the pomodoro clock project that is not covered by the test suite.

"When I first click the element with id="start_stop", the
timer should begin running from the value currently displayed in
id="session-length", even if the value has been incremented or
decremented from the original value of 25."

If the session length is set to 60, it is expected that the timer is displaying a value of "60:00", but this case is not covered by the test mentioned above - or any other test.

Following is a link to a live implementation of a project highlighting the issue. You will notice that all the tests pass, though there is actually a bug where the session length is set to 60, while the timer is displaying a value of "00:00" (also reflected in the screenshot below).

https://abdusabri.github.io/fcc-pomodoro-bug/

Screenshot

image

Suggested fix

If you agree that this case should be covered by the test suite, here is an idea for a suggested fix. If so, i could clean it up and create a proper pull request.

it(`When I first click the element with id="start_stop", the
      timer should begin running from the value currently displayed in
      id="session-length", even if the value has been incremented or
      decremented from the original value of 25.`,
      function() {
        clickButtonsById([startStop]);
        assert.strictEqual(
          getMinutes(
            document.getElementById('time-left').innerText
          ),
          getInputValue(document.getElementById('session-length'))
        );
  
        /*****************/
        /* suggested fix */
        resetTimer();
        clickButtonsById(Array(35).fill(seshPlus)); // Set session length to 60
        
        clickButtonsById([startStop]);
  
        // Another option could be to explicitly test for '60' and '60:00' 
        assert.strictEqual(
          getMinutes(
            document.getElementById('time-left').innerText
          ),
          getInputValue(document.getElementById('session-length'))
        );
        /*****************/
      });
@tbushman

This comment has been minimized.

Copy link
Contributor

@tbushman tbushman commented Jan 6, 2019

@abdusabri It should be fine to add this test. Thanks for your contribution.

@ValeraS

This comment has been minimized.

Copy link
Contributor

@ValeraS ValeraS commented Jan 6, 2019

@abdusabri Why is this an edge case? For example, why is the edge case not equal to 100?

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

@abdusabri abdusabri commented Jan 6, 2019

@tbushman, @ValeraS Thanks for your feedback!

@ValeraS the way I thought about it:

  • According to the project requirements, 60 is the max allowed length - which is covered by another test already. So, 100 or any value higher than 60 would already fail the tests.
  • This is the point where 60 minutes equals 1 hour 😄. And what is actually happening in the example project I linked, is that the time left is formatted like "01:00:00" instead of "60:00"

Now that I look at it, does it make more sense to have this case covered under the test "I can see an element with corresponding id="time-left". NOTE: Paused or running, the value in this field should always be displayed in mm:ss format (i.e. 25:00)." ?

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

@abdusabri abdusabri commented Jan 13, 2019

@ValeraS your feedback will be highly appreciated, so I can be sure to have answered your question before opening a pull request. Thank you!

@ValeraS

This comment has been minimized.

Copy link
Contributor

@ValeraS ValeraS commented Jan 14, 2019

User Story # 17: I should not be able to set a session or break length to > 60.

Our reference project does not follow this. If we decide to check this we must update the project accordingly.

I think a test should try to set 61 and check the value is 60 for the session and the break.

Or we can amend the user story and add a test which tries to set 61 and checks the value is 61.

@tbushman your thoughts on this?

@tbushman

This comment has been minimized.

Copy link
Contributor

@tbushman tbushman commented Jan 14, 2019

@abdusabri This discussion has basically determined that no change is necessary for this particular thing. I hear about other things people suspect are problematic with the pomodoro example and tests. But this change is possibly redundant, in that the user story already informs students of the project's limitation, and the test is in place for that user story. Feel free to open a PR to better demonstrate, without necessitating changing the example project if possible.

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

@abdusabri abdusabri commented Jan 15, 2019

@ValeraS , @tbushman thanks for your feedback and taking the time looking into this issue.

Maybe as @tbushman suggested, opening a PR might better-demonstrate the case i mentioned.

I will open a PR that will not require any changes to freeCodeCamp's example project or user stories.

The PR will amend an existing test case to ensure that the time-left is formatted correctly ("60:00") when the session length is set to 60, which is not currently covered as shown in the screenshot in the issue description above.

@abdusabri abdusabri mentioned this issue Jan 16, 2019
7 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.