Draft
Conversation
squirrelsc
reviewed
Nov 13, 2024
| @@ -0,0 +1,135 @@ | |||
| # Copyright (c) Microsoft Corporation. | |||
Member
There was a problem hiding this comment.
Please put them under the vm_extensions folder, so we can manage vm extension cases easier. You can have a subfolder there.
squirrelsc
reviewed
Nov 13, 2024
| class MDETest(TestSuite): | ||
| def before_case(self, log: Logger, **kwargs: Any) -> None: | ||
| variables = kwargs["variables"] | ||
| self.onboarding_script_sas_uri = variables.get("onboarding_script_sas_uri", "") |
Member
There was a problem hiding this comment.
Can it support non-sas uri? The extra requirement makes the test case skipped in most pipelines.
squirrelsc
reviewed
Nov 13, 2024
| return True | ||
|
|
||
| def _install(self) -> bool: | ||
| if not self.get_mde_installer(): |
Member
There was a problem hiding this comment.
It looks the get_mde_installer return true always. The code logic won't be hit.
squirrelsc
reviewed
Nov 13, 2024
| def verify_mde(self, node: Node, log: Logger, result: TestResult) -> None: | ||
| # Invoking tools first time, intalls the tool. | ||
| try: | ||
| output = node.tools[Mdatp]._check_exists() |
Member
There was a problem hiding this comment.
- Don't call the private method like
_check_exists, callexists. - But when you refer a tool like this, the check/install happens automatically. So, the code can be like
_ = node.tools[Mdatp], and capture the exception if there is.
squirrelsc
reviewed
Nov 13, 2024
| return self._check_exists() | ||
|
|
||
| def onboard(self, onboarding_script_sas_uri: str) -> bool: | ||
| if not self._check_exists(): |
Member
There was a problem hiding this comment.
It doesn't need to check, when you have an instance of the tool, it means the check exists already passed.
squirrelsc
reviewed
Nov 13, 2024
| force_run=True, | ||
| ) | ||
|
|
||
| result.assert_exit_code(include_output=True) |
Member
There was a problem hiding this comment.
Add error message for assertion, so the test case can bring more information, when it's failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is based of the original PR - #3113