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

Fixup the unit.client_test.LocalClientTestCase.test_cmd_subset from #35720 #35753

Merged
merged 2 commits into from
Aug 25, 2016

Conversation

rallytime
Copy link
Contributor

What does this PR do?

The bug fix for #20575, which is located in PR #35720, caused a test failure in the unit.client_test tests that wasn't caught until later. This PR fixes that test.

Background information on why I am fixing it this way, after confirming with @thatch45 :

  • The ret option in this function actually stands for returner and should just be a pass-through and not overwritten in the cmd_subset function as was originally occurring in the function before the fix in fix 20575, make subset really return random subset #35720. Therefore, the expected return in this unit test should be ret=''.
  • I am using a try/except on each of these tests, because now that the cmd_subset function actually returns a random subset of minions, sometimes the sub=1 function operates on minion2 and sometimes it is minion1. The same goes for the list of minions for sub=10. Since this assert is against a list, sometimes the list of minions will be called in differing orders.

What issues does this PR fix or reference?

Refs #35720

Previous Behavior

Test failing on 2016.3 branch.

New Behavior

Test will pass now.

Tests written?

Yes

Please review Salt's Contributing Guide for best practices.

@hu-dabao
Copy link
Contributor

@rallytime
Good job, thanks for fixing the tests. 👍

@rallytime
Copy link
Contributor Author

@hu-dabao No problem! Thanks for fixing the code. :D

@thatch45 thatch45 merged commit b3f6367 into saltstack:2016.3 Aug 25, 2016
@rallytime rallytime deleted the fix-client-unit-test branch August 25, 2016 15:55
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