Skip to content

Refactor tests/unit/states/test_sysctl.py#58746

Merged
dwoz merged 4 commits intosaltstack:masterfrom
asomers:test_sysctl_wisely
Nov 12, 2020
Merged

Refactor tests/unit/states/test_sysctl.py#58746
dwoz merged 4 commits intosaltstack:masterfrom
asomers:test_sysctl_wisely

Conversation

@asomers
Copy link
Copy Markdown
Contributor

@asomers asomers commented Oct 16, 2020

What does this PR do?

The old test combined several different independent commands into a
single obese test case, with overlapping local variables.

Break it into small individual test cases. Also, fix some of the mocks,
which only triggered correct behavior by coincidence.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@asomers asomers requested a review from a team as a code owner October 16, 2020 15:41
@asomers asomers requested review from krionbsd and removed request for a team October 16, 2020 15:41
@ghost ghost requested a review from garethgreenaway October 16, 2020 15:41
@asomers
Copy link
Copy Markdown
Contributor Author

asomers commented Oct 16, 2020

Thanks @krionbsd . Although, I already had such a commit ready to go. In the future, should I force-push stylistic fixes ASAP rather than wait for the tests to finish?

@krionbsd
Copy link
Copy Markdown
Contributor

Thanks @krionbsd . Although, I already had such a commit ready to go. In the future, should I force-push stylistic fixes ASAP rather than wait for the tests to finish?

Yeah, that would make sense to reduce total CI run/time

@asomers
Copy link
Copy Markdown
Contributor Author

asomers commented Oct 16, 2020

And @krionbsd do you prefer that I push style fixes as separate commits, or that I amend the original commit and force-push?

@krionbsd
Copy link
Copy Markdown
Contributor

And @krionbsd do you prefer that I push style fixes as separate commits, or that I amend the original commit and force-push?

@asomers I think amending the original commit and force-push is ok

@asomers
Copy link
Copy Markdown
Contributor Author

asomers commented Oct 16, 2020

The test failure in ci/py3/ubuntu1604/m2crypto is definitely unrelated to this PR, because this PR only affects tests in a different file.

@asomers
Copy link
Copy Markdown
Contributor Author

asomers commented Oct 16, 2020

Again ci/py3/ubuntu1604/m2crypto failed for the exact same reason. @krionbsd do you have any idea why?

s0undt3ch
s0undt3ch previously approved these changes Oct 16, 2020
krionbsd
krionbsd previously approved these changes Oct 16, 2020
@krionbsd
Copy link
Copy Markdown
Contributor

Again ci/py3/ubuntu1604/m2crypto failed for the exact same reason. @krionbsd do you have any idea why?

That's a magic :)

The old test combined several different independent commands into a
single obese test case, with overlapping local variables.

Break it into small individual test cases.  Also, fix some of the mocks,
which only triggered correct behavior by coincidence.
@asomers asomers dismissed stale reviews from krionbsd and s0undt3ch via d6f604c October 18, 2020 14:49
@asomers asomers force-pushed the test_sysctl_wisely branch from a878193 to d6f604c Compare October 18, 2020 14:49
@asomers
Copy link
Copy Markdown
Contributor Author

asomers commented Oct 18, 2020

Rebased and squashed

@asomers asomers requested a review from krionbsd November 4, 2020 15:03
@asomers
Copy link
Copy Markdown
Contributor Author

asomers commented Nov 4, 2020

The most recent build failure looks unrelated to this PR.

@s0undt3ch s0undt3ch changed the title Refactor tests/unit/states/test_sysctl.py WIP Refactor tests/unit/states/test_sysctl.py Nov 9, 2020
@s0undt3ch s0undt3ch changed the title WIP Refactor tests/unit/states/test_sysctl.py Refactor tests/unit/states/test_sysctl.py Nov 9, 2020
@krionbsd krionbsd added the v3002.2 vulnerable version label Nov 9, 2020
@dwoz dwoz merged commit 9457fd4 into saltstack:master Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v3002.2 vulnerable version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants