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

Test Segment tracking after user creation #357

Merged

Conversation

vidmantas
Copy link
Contributor

@vidmantas vidmantas commented Oct 16, 2016

Tries to implement segment tracking testing requested in #350

Used some ideas from http://blog.plataformatec.com.br/2015/10/mocks-and-explicit-contracts/

I don't think that message sending to self is useful in development, so I've created a separate implementation to be plugged in for testing only.

@vidmantas vidmantas force-pushed the test-segment-analytics-tracking branch from 77b8f65 to 9c821c1 Compare October 16, 2016 11:41
Copy link
Contributor

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

This is fantastic. Good work @vidmantas!

@joshsmith
Copy link
Contributor

Did you want to merge this now or test the other calls in the context of this PR? I'd be fine with either.

@begedin
Copy link
Contributor

begedin commented Oct 17, 2016

This is what I imagined we'd have. Great work, @vidmantas

@vidmantas
Copy link
Contributor Author

I've added a test for another users controller method, however before proceeding with other occurrences of Segment tracking wanted to raise a question on organizing tests:

now we have pretty big chunks of repetitive "setup" part before assertions on some create and update test cases (e.g. when valid data is passed, lines 75-83, 95-102) . Let's try to organize them using named setups? If you want, I can provide a separate PR as a suggestion (in this case I think it would make sense to merge this one and continue for the rest of Segment calls as separate PR after there's decision on this question)

@begedin
Copy link
Contributor

begedin commented Oct 17, 2016

@vidmantas We already had a few attempts at reducing test boilerplate, especially with creating records/payloads, but weren't happy with the results. Feel free to make a PR with your suggestion. I have a feeling it will be well received.

As for this, we can consider it ready, so squash/rebase and merge when you get the chance. If you also don't mind, keep the referrenced issue open and add a comment saying what else is needed (as in "the infrastructure for this has been merged, now it's just a matter of writing the individual tests."), with maybe links to the relevant code.

@joshsmith
Copy link
Contributor

@vidmantas I second what @begedin said!

@vidmantas
Copy link
Contributor Author

@begedin @joshsmith alright. I just don't have write access to merge, so please do that instead of me and I'll proceed as discussed ;)

@joshsmith
Copy link
Contributor

Can you squash? Then we can merge.

@vidmantas vidmantas force-pushed the test-segment-analytics-tracking branch from 949e9f0 to 2368542 Compare October 17, 2016 16:24
@vidmantas
Copy link
Contributor Author

@joshsmith ah, right, sorry. Done now :)

@joshsmith joshsmith merged commit 593f659 into code-corps:develop Oct 17, 2016
@joshsmith
Copy link
Contributor

🙌 !

@vidmantas vidmantas deleted the test-segment-analytics-tracking branch October 18, 2016 08:01
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.

3 participants