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 initial tests to auth command #83

Merged
merged 23 commits into from Jan 2, 2018
Merged

Add initial tests to auth command #83

merged 23 commits into from Jan 2, 2018

Conversation

lricoy
Copy link
Collaborator

@lricoy lricoy commented Dec 29, 2017

No description provided.

@danthareja
Copy link
Owner

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
Copy link
Collaborator

aurelienshz commented Dec 31, 2017

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
Copy link
Collaborator

aurelienshz commented Dec 31, 2017

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 🙂 ).

@lricoy
Copy link
Collaborator Author

lricoy commented Jan 2, 2018

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
Copy link
Owner

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
Copy link
Collaborator Author

lricoy commented Jan 2, 2018

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

@lricoy lricoy merged commit 204618b into master Jan 2, 2018
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.

None yet

3 participants