Skip to content

Conversation

@tuttinator
Copy link

Referencing to #6

This is my first approach at adding some tests. Not ready to merge yet, and I'll keep adding to this PR as I go - but I thought I'd open it early to allow you to provide feedback instead of one large PR all at once.

I've added a Rakefile as Travis CI will just run rake by default.

I've also thought about using VCR to stub HTTP requests - so that testing API calls don't hit real servers.

Feel free to let me know if there are any stylistic changes you prefer.

config.password = password
end

expect(DocumentCloud.client.instance_variable_get(:@email)).to eq CGI.escape(email)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessarily the nicest way to test this. Reaching in with instance_variable_get is something of an antipattern, and I'll try other approaches.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a test, so if it works and handles the case the test is for, im good with it :)

@mileszim
Copy link
Owner

Honestly maintenance of this is a low priority so whatever work you can contribute will be awesome. Feel free to use whatever tools you deem best, I trust you know what will work well since you've been using it frequently. I'm happy to merge when you think it's ready.

@mileszim
Copy link
Owner

Tests are passing @tuttinator, it's been far too long and I apologize getting back to you. Do you think this is merge ready?

Copy link
Owner

@mileszim mileszim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tuttinator I'm going to work off this branch to get tests in before i merge this and start updating dependencies, is that alright with you?

@mileszim
Copy link
Owner

Great work! I borrowed your scaffold and am going from there. Thank you! Closing this in favor of #17

@mileszim mileszim closed this Aug 12, 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.

2 participants