-
Notifications
You must be signed in to change notification settings - Fork 53
Fixing faulty logic in InstallFunctionAppDependencies and adding unit test #250
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
Fixing faulty logic in InstallFunctionAppDependencies and adding unit test #250
Conversation
… DependencyManager.cs to accommodate for unit test.
…endencies as well as a successful module download.
… DependencyManager.cs to accommodate for unit test.
…endencies as well as a successful module download.
| var message = string.Format(PowerShellWorkerStrings.ModuleHasBeenInstalled, moduleName, version); | ||
| var filePath = Path.Join(path, TestFileName); | ||
| File.WriteAllText(filePath, message); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need a file? Why not just keep a list of messages in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep the module name and version in memory. Test case updated.
| } | ||
|
|
||
| [Fact] | ||
| public void TestManagedDependencyRetryLogic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestManagedDependencyRetryLogic [](start = 20, length = 31)
Consider adding a test for the case when the retry logic kicks in and succeeds. In order to do simulate that, I would suggest passing a number to TestDependencyManager instead of just a boolean ShouldThrow (something like ShouldThrowCount).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks for the suggestion.
I have added a new test case that succeeds after two failed attempts.
@AnatoliB: Could you please take another look?
…hell-worker into retry-logic-and-unit-test-2
…installation run if the PSGallery could not be reached, but continue if previous dependencies exist.
…tinue if the PSGallery could not be reached but a previous installation of managed dependencies exists.
This PR contains the following changes:
After this code change, in the InstallFunctionAppDependencies, we perform two operations:
If an issue is encountered in either (1) or (2) after three retries, the exception will be caught and wrapped in a DependencyInstallationException for post processing. This exception is expose via DependencyManager.DependencyError.
Added unit test to validate installing the function app dependencies and the installation retry logic. For this, calls to Save-Module are being mocked.
Installation flow for first install vs update
I added logic to dependency manager to fail in the first dependencies installation run if the PSGallery could not be reached, but continue if previous dependencies exist.
Lastly, I added unit tests to validate that a function app execution should continue if the PSGallery could not be reached but a previous installation of managed dependencies exists. For this, I've refactored code from
DependencyManagementUtilstoDependencyManagerto be able to mock calls to the PSGallery to retrieve the latest module version.