search

danthareja / node-google-apps-script

[DEPRECATED - use clasp instead] The easiest way to develop Google Apps Script projects
MIT License
353 stars 70 forks source link

Add initial tests to auth command #83

Closed lricoy closed 6 years ago

danthareja commented 6 years ago

Nice work @lricoy! So glad to finally have some tests. I'll try to review this one over the weekend.

Also curious to see if @aurelienshz has any feedback.

aurelienshz commented 6 years ago

In a close future I feel like we should write a decent mock for the fs module, since we are testing a lot of things related to reading from / writing to the disk, and actually hitting the disk can be slow and unreliable. But I think for now using /tmp for the auth module is fine (since we only write a single file).

This looks pretty nice, but when running the test it hangs after the end and never gives back the prompt. Have you encountered this problem?

aurelienshz commented 6 years ago

Apparently, the issue is that the server that gets started by miniOauthServer hangs indefinitely even after the end of the test. There is a fix in #85, in which I mock miniOauthServer, and also mock the function from googleapis that allow to hit Google's token endpoint, and this allows us to perform assertions on the obtained token (and also get rid of the timeout with 1000ms delay :slightly_smiling_face: ).

lricoy commented 6 years ago

Nicely done @aurelienshz I agree with mocking the fs module and thanks for the fix. Didn't have the time to do it at first. The PR was manly to get some early feedback from you guys.

I will take a look at writing somewhat functional tests instead of unit ones so we can have it running on a CI server and may allow it to hit the filesystem and google's servers.

danthareja commented 6 years ago

Sorry @lricoy, I didn't find time to review in depth. It looks @aurelienshz has your back on the review (thanks bud!)

I have been following the conversation and I do like your suggestion to stick to functional/integration tests over unit for now, for the purpose of testing the whole module with a minimal amount of tests in the beginning.

Great job so far!

lricoy commented 6 years ago

Hey @danthareja . No worries. Yeah, I think we are all in sync. Will merge this one so we can move forward.