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

system.reboot instead of system.restart #60111

Merged
merged 5 commits into from Jun 2, 2021
Merged

Conversation

twangboy
Copy link
Contributor

@twangboy twangboy commented Apr 29, 2021

What does this PR do?

Uses system.reboot instead of the non-existent system.restart

Need to finish up the tests

What issues does this PR fix or reference?

Fixes: #59424

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@twangboy twangboy requested a review from a team as a code owner April 29, 2021 22:58
@twangboy twangboy requested review from xeacott and removed request for a team April 29, 2021 22:58
@twangboy twangboy changed the title [WIP] system.reboot instead of system.restart system.reboot instead of system.restart May 25, 2021
@twangboy twangboy added Silicon v3004.0 Release code name Windows labels May 25, 2021
"Success": True,
}

mock_reboot = MagicMock(return_value=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking) FWIW, a great way to avoid the problem of non-existent functions is to use create_autospec. Sometimes it's no bueno, but usually:

mock_reboot = create_autospec(return_value=True)

Found wherever other useful Mocks are sold!


mock_reboot = MagicMock(return_value=True)
with patch.object(
win_servermanager, "_pshell_json", return_value=mock_out
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion(non-blocking) Then in cases like this, adding autospec=True to the patch call, will ensure that the mock looks like the real _pshell_json.

}
],
"ExitCode": 0,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): note that when the same data is found in multiple tests, especially if the same function is mocked to return that same data each time, that's an ideal place to use a pytest fixture.

@sagetherage sagetherage added Aluminium Release Post Mg and Pre Si point-release minor release and removed Silicon v3004.0 Release code name labels Jun 2, 2021
@sagetherage sagetherage added this to In progress in Windows via automation Jun 2, 2021
@sagetherage sagetherage moved this from In progress to Review / QA in Windows Jun 2, 2021
@Ch3LL Ch3LL merged commit 40d2484 into saltstack:master Jun 2, 2021
Windows automation moved this from Review / QA to Done Jun 2, 2021
@sagetherage sagetherage added Silicon v3004.0 Release code name and removed Aluminium Release Post Mg and Pre Si point-release minor release labels Jun 2, 2021
@twangboy twangboy deleted the fix_59424 branch March 23, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Windows
  
Done
Development

Successfully merging this pull request may close these issues.

[BUG] win_servermanager: KeyError: 'system.restart'
5 participants