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

Gem integration test #33691

Merged
merged 13 commits into from Jun 6, 2016
Merged

Conversation

justinta
Copy link

@justinta justinta commented Jun 1, 2016

What does this PR do?

Adds integration tests for the gem module.

@cachedout Any critique here would be appreciated.

Tests written?

Yes

gem.sources_remove
'''
sources_list = self.run_function('gem.sources_list')
source = 'http://gems.github.com'
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems subject to change. I don't think this block is necessary.

@cachedout
Copy link
Contributor

Overall, I think this is starting to look pretty good. My main concern with these types of tests is that they tend to flake when the upstream service isn't online. Ultimately, I would like to move in a direction where we can test this stuff without relying on external services but this is fine for now.

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 2, 2016
@justinta
Copy link
Author

justinta commented Jun 2, 2016

Sounds good. I'll make those changes. I'm not sure how to abstract the test away from the upstream service, unless we mock that potential output, but then it seem like we're just testing the mock, instead. Anyway, just thinking out loud.

@cachedout
Copy link
Contributor

@jtand Yeah, you've got the issue exactly. There's a part of me that wants to just build out a shadow infrastructure to handle all of this but I somehow doubt that's going to be any more reliable. I have yet to really come up with a solution for this problem that I like.

@justinta
Copy link
Author

justinta commented Jun 2, 2016

@cachedout Hmmm, I think something that would check to make sure the external resource is live before running any of the tests would help alleviate that issue some.

@cachedout
Copy link
Contributor

That's a really good idea. A skipIf that just points to a function that does the checking is a terrific idea. I like it!

@justinta
Copy link
Author

justinta commented Jun 2, 2016

Yeah. I'll look into it and pm you with what I come up with.

@cachedout
Copy link
Contributor

Cool. Should be easy.

@cachedout
Copy link
Contributor

@jtand Lint fixup needed, please.

@justinta
Copy link
Author

justinta commented Jun 3, 2016

On it. Local lint missed it for some reason.

@cachedout cachedout merged commit 7fdfbe9 into saltstack:2015.8 Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants