hack4impact-upenn / transcribe4all

Painless speech-to-text transcription
MIT License
42 stars 10 forks source link

Function to download mp3 file locally #24

Closed sandlerben closed 8 years ago

sandlerben commented 8 years ago

See #22

sandlerben commented 8 years ago

Made this so code review would be easier

yoninachmany commented 8 years ago

Thanks! Should I create mock tests? Not really sure why I would mock http.Get or io.Copy

sandlerben commented 8 years ago

If you don't mock those functions the code will make an actual get request (meaning our tests would require internet) and copy an actual file.

sandlerben commented 8 years ago

Plus that would introduce non-determinism into our tests.

yoninachmany commented 8 years ago

Right now, determinism in mocking comes from mocking smtp functions from Go library, which ideally should be working, but I guess I understand that we should test them.

I guess my question is: shouldn't we continuously test the real thing - sending an actual email or downloading an actual url?

sandlerben commented 8 years ago

Thanks! Should I create mock tests? Not really sure why I would mock http.Get or io.Copy

You would mock these things so that our application wouldn't copy to a file or make an http get request during test. I'm actually okay with not mocking io.Copy as long as the file is deleted as part of the clean up.

On Friday, March 4, 2016, Yoni Nachmany notifications@github.com wrote:

Thanks! Should I create mock tests? Not really sure why I would mock http.Get or io.Copy

— Reply to this email directly or view it on GitHub https://github.com/hack4impact/audio-transcription-service/pull/24#issuecomment-192363779 .

Benjamin Sandler University of Pennsylvania | Class of 2018 Candidate for BS in Computer Science & BA in Cognitive Science 954.478.6689 | ben1sandler@gmail.com

sandlerben commented 8 years ago

I guess my question is: shouldn't we continuously test the real thing - sending an actual email or downloading an actual url?

No. We should assume that when we make a call to a library function like smtp.Send or http.Get that those library functions will magically work correctly. We should not test external dependencies as part of unit tests.

On Friday, March 4, 2016, Yoni Nachmany notifications@github.com wrote:

Right now, determinism in mocking comes from mocking smtp functions from Go library, which ideally should be working, but I guess I understand that we should test them.

I guess my question is: shouldn't we continuously test the real thing - sending an actual email or downloading an actual url?

— Reply to this email directly or view it on GitHub https://github.com/hack4impact/audio-transcription-service/pull/24#issuecomment-192409734 .

Benjamin Sandler University of Pennsylvania | Class of 2018 Candidate for BS in Computer Science & BA in Cognitive Science 954.478.6689 | ben1sandler@gmail.com

yoninachmany commented 8 years ago

I'm actually okay with not mocking io.Copy as long as the file is deleted as part of the clean up.

To clarify: there are three Go library functions that I am using: os.Create, http.Get, and io.Copy

If you don't think I should mock io.Copy, should I still mock os.Create and http.Get?

If yes, then "clean-up" occurs in the tests, right? Thanks.

sandlerben commented 8 years ago

I am okay with you only mocking Get but you might find it easier to mock them all. If you do create a file, you may want the test to read it and check its contents.

On Sat, Mar 12, 2016, 11:08 AM Yoni Nachmany notifications@github.com wrote:

I'm actually okay with not mocking io.Copy as long as the file is deleted as part of the clean up.

To clarify: there are three Go library functions that I am using: os.Create, http.Get, and io.Copy

If you don't think I should mock io.Copy, should I still mock os.Create and http.Get?

If yes, then "clean-up" occurs in the tests, right? Thanks.

— Reply to this email directly or view it on GitHub https://github.com/hack4impact/audio-transcription-service/pull/24#issuecomment-195768203 .

yoninachmany commented 8 years ago

Working through this issue: screen shot 2016-03-13 at 7 42 47 pm

sandlerben commented 8 years ago

I have no idea what that error means. What the heck is causing that?