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

add unit test for Time view #428

Merged
merged 1 commit into from
Oct 11, 2020
Merged

Conversation

bobsany16
Copy link
Contributor

@bobsany16 bobsany16 commented Oct 10, 2020

Added a unit test for Time view related to issue #124

Test cases (very basic):

  • if Time renders all the necessary component
  • if Time calls getNow() which then calls setInterval()

Will need to:

  • test if the .date and .time elements in Time view renders actual dates and time ( I was having trouble with this)

However, coverage for Time is currently 100%.

Screen Shot 2020-10-10 at 3 17 37 PM

@bobsany16
Copy link
Contributor Author

@jayehernandez Please let me know if I should mark this as draft if you think I should keep working on it!

@jayehernandez
Copy link
Owner

jayehernandez commented Oct 11, 2020

Thanks for the progress on this @bobsany16! Were you able to troubleshoot why .date and .time doesn't render the actual date and time? If we can't find the solution for now, I'm okay with what's tested so far too.

@bobsany16
Copy link
Contributor Author

@jayehernandez no, thank you for this project!! I've learned so much about testing 👌and still have more to learn!

I tried commenting out the setInterval() so that the getNow() just directly renders the date and time only once and when I tested it if they match a Date() defined in the test, they all passed. However, when I put the setInterval() back in, the test returns "" for .date and .time so I am still troubleshooting why.

@jayehernandez
Copy link
Owner

Oh got it! Are you good with merging this for now? Then just creating a separate PR when the issue about the "" for .date and .time is found?

Since there's already solid foundation here resulting to 100% test coverage. With the rate that PRs are going now, I'm just worried that someone else might also test out the Time component as a duplicate.

@bobsany16
Copy link
Contributor Author

oh cool! Yes, you can merge it! Im more than happy 😊. I'll just make a separate PR when I solve that issue! Thank you 🙏

@jayehernandez
Copy link
Owner

Great!! Thanks for working on this again! 🥳

@all-contributors please add @bobsany16 for testing

@jayehernandez jayehernandez merged commit 492777d into jayehernandez:master Oct 11, 2020
@allcontributors
Copy link
Contributor

@jayehernandez

I've put up a pull request to add @bobsany16! 🎉

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.

2 participants