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

Additional Logging for Flakey Workspace Cleanup Test and removal of Sync over Async while deleting a workspace #740

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

nathanwoctopusdeploy
Copy link
Contributor

@nathanwoctopusdeploy nathanwoctopusdeploy commented Dec 14, 2023

Background

Adding some more logging to a flakey test WhenScriptServiceIsRunning_ThenWorkspaceIsNotDeleted e.g. https://build.octopushq.com/buildConfiguration/TeamFireAndMotion_OctopusTentacleNet48_SensibleDefaultsChainFullChain/10569176?hideTestsFromDependencies=false&hideProblemsFromDependencies=false&expandBuildTestsSection=true&expandBuildProblemsSection=true&expandBuildDeploymentsSection=false

Removed a sync call that then did a Wait on an async call as well can be fully async now.

[sc-66742]

How to review this PR

Quality ✔️

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

Additional test logging to try and diagnose flakey behavour.
Copy link

This pull request has been linked to Shortcut Story #66742: Tentacle Flakey Tests.

logger.Information("****** CLIENT AND TENTACLE DISPOSE CALLED *****");
logger.Information("* Subsequent errors should be ignored *");
logger.Information("****** ****** ****** ****** ****** ****** ******");
var banner = new StringBuilder();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly easier to read log
image

Directory.Exists(startScriptWorkspaceDirectory).Should().BeFalse("Workspace should be naturally cleaned up after completion");
runningScriptResult.LogExecuteScriptOutput(Logger);

runningScriptResult.ScriptExecutionResult.ExitCode.Should().Be(0, "Script should have completed successfully");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to ensure everything works in the test and there is not another side effect causing the test to fail

@@ -82,11 +82,6 @@ public string ResolvePath(string fileName)
return path;
}

public void Delete()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing Sync code path that does a Wait on an Async call as we can be fully async now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving FileSystem.DeleteDirectory sync method as it is used in other places in the tests. Future cleanup task.

@nathanwoctopusdeploy nathanwoctopusdeploy marked this pull request as ready for review December 14, 2023 04:42
@nathanwoctopusdeploy nathanwoctopusdeploy requested a review from a team as a code owner December 14, 2023 04:42
@nathanwoctopusdeploy nathanwoctopusdeploy changed the title Flakey Workspace Cleanup Test Additional Logging for Flakey Workspace Cleanup Test and removal of Sync over Async while deleting a workspace Dec 14, 2023
{
public static class ScriptExecutionResultExtensionMethods
{
public static void LogExecuteScriptOutput(this (ScriptExecutionResult ScriptExecutionResult, List<ProcessOutput> ProcessOutput) result, ILogger logger)
Copy link
Contributor Author

@nathanwoctopusdeploy nathanwoctopusdeploy Dec 14, 2023

Choose a reason for hiding this comment

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

image

@nathanwoctopusdeploy nathanwoctopusdeploy merged commit be322a0 into main Dec 14, 2023
2 checks passed
@nathanwoctopusdeploy nathanwoctopusdeploy deleted the sast/flakey-workspace-cleanup-tests branch December 14, 2023 06:27
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.

2 participants