-
Notifications
You must be signed in to change notification settings - Fork 213
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
Betty unit test. #88
base: dev
Are you sure you want to change the base?
Betty unit test. #88
Conversation
Add DateTime executor unit test. |
I think that it is not very good approach when you add functionality just to make test pass. |
|
|
|
|
Nope. The purpose of these test suites are not checking the correctness of specified module methods. The correct purpose is to see that does the specified module return our preferred command or not? |
|
@pickhardt need you to participate here. |
Yes, but you've said what I mean.In your new tests you've checked functionality.But in previous version you've not.So my comment was about that.Now your tests seem to be ok.
You test module here.So as much aspects you'd test as better your test suite is.I understand that you can't write test on help methods, but all other methods module functionality depends on are better to cover.Maybe not now, but it is good practice when your test coverage is high. |
The new tests you wrote are pretty good, @Ch0c0late. @igorrKurr's approach to testing is better because he tests the actual command response, and because he doesn't add functionality to the actual code just for the purposes of testing. I can't automatically merge this pull request due to merge conflicts. Probably because I just merged this one https://github.com/pickhardt/betty/pull/101/files Can you fix the merge conflicts? |
@pickhardt Should I merge the upstream change(those the @igorrKurr wrote) into this branch? |
Yeah, you'll have to merge the current dev into your branch. Sent from my Android.
|
@pickhardt Thanks. I'm going to merge it. |
@pickhardt Merged dev into betty unit-test branch. Hope it works correctly. |
@pickhardt Wrote test for all of betty's modules. |
These integration tests look great! In terms of test style, can I get someone to look at #129 |
@brysgo Took a look at it. Looks good. |
Getting these tests in is high priority for me. I'm going to merge in my fun tests and we can use the test configuration from it here. |
If you rebase this off the current dev branch, you can run |
Add OS executor test suite.