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

fix: assert time-left is formatted correctly #772

Merged
merged 1 commit into from Jan 31, 2019

Conversation

@abdusabri
Copy link
Contributor

abdusabri commented Jan 16, 2019

Pre-Submission Checklist

  • Your pull request targets the master branch of freeCodeCamp/testable-projects-fcc
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/signin-issue)
  • You have only one commit (if not, squash them into one commit).
  • Your changes have been tested either locally or using a newly created CDN based on your fork's testable-projects-fcc/build/bundle.js file
  • Working changes have been added to the build by running npm run build

Type of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Add new translation (feature adding new translations)

Checklist:

  • Tested changes locally.
  • Closes currently open issue: Closes #755

Description

This PR fixes an assertion issue for the pomodoro clock testing project. The issue was that all tests pass even if the time-left was incorrectly formatted as 00:00 instead of 60:00 when session length is set to 60

The following screenshots demonstrate the fix

image

image

An example project before the fix can be found here: https://abdusabri.github.io/fcc-pomodoro-bug/
An example project after the fix can be found here: https://abdusabri.github.io/fcc-pomodoro-fix/

@tbushman

This comment has been minimized.

Copy link
Contributor

tbushman commented Jan 29, 2019

@abdusabri I was trying both the links you posted, and noticed the tests pass for both. Did you update the first link listed above?

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Jan 29, 2019

Thanks for checking @tbushman

I didn't change either of the links

https://abdusabri.github.io/fcc-pomodoro-bug/ -> uses the CDN version of the tests, and all tests pass

https://abdusabri.github.io/fcc-pomodoro-fix/ -> uses a production build of this PR's tests, and one test fails (screenshots in this PR's description)

Would it help if i send a few screenshots comparing them (showing the urls and more details)?

@tbushman

This comment has been minimized.

Copy link
Contributor

tbushman commented Jan 29, 2019

@abdusabri Mainly I checked the first link, expecting it to fail. Sorry. I think we're probably good. Thanks for your patience.

@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Jan 29, 2019

@tbushman No worries! 🙂 and thanks for reviewing!

@tbushman

This comment has been minimized.

Copy link
Contributor

tbushman commented Jan 31, 2019

@abdusabri I noticed we're using a RegExp on line 83. I think it would be safest to emulate that style of assertion.

fix: assert time-left is formatted correctly

fix: assert time-left is formatted correctly, use getMinutes
@abdusabri abdusabri force-pushed the abdusabri:fix/assert-time-left-format branch from cd4184a to 7d0a548 Jan 31, 2019
@abdusabri

This comment has been minimized.

Copy link
Contributor Author

abdusabri commented Jan 31, 2019

@tbushman Thanks for the feedback

I've updated the code to use getMinutes function, which uses the RegExp, and I've rebased and squashed the commits.

@tbushman

This comment has been minimized.

Copy link
Contributor

tbushman commented Jan 31, 2019

Alrighty, @abdusabri ! Thanks for your contribution. Congratulations on your first contribution to this repository. LGTM!

@tbushman tbushman merged commit 783be40 into freeCodeCamp:master Jan 31, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

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