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

Fix specs to be compatible with forked repositories #152

Merged
merged 2 commits into from
Jan 5, 2019

Conversation

mrmanc
Copy link
Contributor

@mrmanc mrmanc commented Jan 4, 2019

Tests were coupled to a specific origin meaning that tests against forks would fail. I’ve replaced the test that checks the output of git_remote_url is okay into a test to check git_remotes returns something, and another to check that git_remote_url correctly interprets the output from git_remotes.

I’ve also corrected the test doubles in later tests where (as part of #147) I had copied an incorrect bit of stubbing of git_remote_url. The stubbing appeared to be mimicking git_remotes instead. The test seemed to be passing by coincidence before. The stubbed output now matches what the function actually does.

Tests were coupled to a specific origin meaning that tests against forks would fail. I’ve replaced the test that checks the output of `git_remote_url` is okay into a test to check `git_remotes` returns something, and another to check that `git_remote_url` correctly interprets the output from `git_remotes`.

I’ve also corrected the test doubles in later tests where (as part of jekyll#147) I had copied an incorrect bit of stubbing of `git_remote_url`. The stubbing appeared to be mimicking `git_remotes` instead. The test seemed to be passing by coincidence before. The stubbed output now matches what the function actually does.
@ashmaroli
Copy link
Member

@mrmanc Can you please explain why you made all those whitespace changes..?

@mrmanc
Copy link
Contributor Author

mrmanc commented Jan 4, 2019

Yikes—I didn’t spot that I’d accidentally triggered my IDE’s reformatting functionality. That should be resolved now.

@ashmaroli
Copy link
Member

The diff looks a lot better now. Thanks
Now all that's left is to rename the PR title.. hmm.. 🤔

P.S. Ignore the failing tests in this context

@mrmanc mrmanc changed the title Fix #148 by stubbing git remote Make sure tests compatible with forked repositories Jan 4, 2019
@mrmanc
Copy link
Contributor Author

mrmanc commented Jan 4, 2019

Now all that's left is to rename the PR title.. hmm.. 🤔

Oh, not sure what the convention is. I’ve made it more descriptive based on looking at some closed PR titles. Any better?

@ashmaroli ashmaroli changed the title Make sure tests compatible with forked repositories Fix specs to be compatible with forked repositories Jan 4, 2019
@ashmaroli
Copy link
Member

Don't worry about it. I've renamed it.

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@ashmaroli ashmaroli added the tests label Jan 4, 2019
@ashmaroli ashmaroli requested review from parkr and a team January 4, 2019 13:55
@parkr
Copy link
Member

parkr commented Jan 5, 2019

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit f3db026 into jekyll:master Jan 5, 2019
jekyllbot added a commit that referenced this pull request Jan 5, 2019
@parkr
Copy link
Member

parkr commented Jan 5, 2019

Thank you so much!!

@mrmanc mrmanc deleted the fix-hard-coded-git-remote branch January 7, 2019 11:17
@mrmanc
Copy link
Contributor Author

mrmanc commented Jan 7, 2019

You’re welcome, I’ve enjoyed these contributions 👍

@jekyll jekyll locked and limited conversation to collaborators Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants