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

Fix flakey tentacle command line tests #737

Conversation

sburmanoctopus
Copy link
Contributor

@sburmanoctopus sburmanoctopus commented Dec 12, 2023

[sc-65398]

Background

The TentacleCommandLineTests.ShouldLogStartupDiagnosticsToInstanceLogFileOnly test was flaky. This needed to be fixed.

Results

Before

The test would create an instance of Tentacle by running Tentacle.exe (setting everything up). Then, while this instance was running, it would run the show-thumbprint command against the same executable, creating a second instance.

What we believe was causing the failure was that the first instance was writing to the log file while the second instance was starting up. This would, lock the log file and prevent the second instance from writing logs.

This test does its assertion by looking at the log file. Since the second instance logs were not being logged, it would therefore fail.

One would expect the second instance to throw an error when file locking occurs. However, we have configured NLog with throwExceptions="false", so if any errors are thrown while logging, it will simply not log and continue as if nothing has happened. This is great for customers, but not for finding test issues.

After

To solve this, we stop the first instance of the running tentacle before we attempt to run the command. This will prevent the file lock, and will therefore allows the logs to be populated as expected.

To make the other tests in this file safer, we also stopped the initial Tentacle instance for each test that also ran a second instance over the first.

To help diagnose future issues, we have also set throwExceptions="true" for tests only. We proved this does indeed work with this test run (where we purposely locked the file)

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.

Copy link

This pull request has been linked to Shortcut Story #65398: Fix flakey tentacle command line tests.

@sburmanoctopus sburmanoctopus changed the title Causing failure if logging cannot succeed Fix flakey tentacle command line tests Dec 12, 2023
@@ -404,10 +408,12 @@ public async Task ListInstancesCommandJson(TentacleConfigurationTestCase tc)
public async Task ShouldLogStartupDiagnosticsToInstanceLogFileOnly(TentacleConfigurationTestCase tc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the test that was flaky

@sburmanoctopus sburmanoctopus marked this pull request as ready for review December 12, 2023 22:17
@sburmanoctopus sburmanoctopus requested a review from a team as a code owner December 12, 2023 22:17
@sburmanoctopus sburmanoctopus force-pushed the sast/FixingShouldLogStartupDiagnosticsToInstanceLogFileOnly branch from 1714600 to ef2570a Compare December 14, 2023 02:04
@sburmanoctopus
Copy link
Contributor Author

FYI, turns out this was not the primary reason for the flaky test.

The main reason was that when Tentacle starts up, it checks to see if it CanLoadCurrentInstance before configuring logging.

If that fails, it will fall back to logging into the default log file (that all tentacles log into). This is what happened when the logs were not showing up.

CanLoadCurrentInstance was claiming to not be able to load an instance because it loads all instances before it tries to find the instance we are looking for. So when another test is manipulating that directory (e.g. deleting instances), we may encounter an exception from that one unrelated instance.

This PR will solve that issue for tests.

Copy link
Contributor

@nathanwoctopusdeploy nathanwoctopusdeploy left a comment

Choose a reason for hiding this comment

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

LGTM

…causing a test to fail that we did not think was worth spending time fixing
@sburmanoctopus sburmanoctopus force-pushed the sast/FixingShouldLogStartupDiagnosticsToInstanceLogFileOnly branch from 34289eb to b25550c Compare December 14, 2023 21:44
@sburmanoctopus
Copy link
Contributor Author

sburmanoctopus commented Dec 14, 2023

We decided to remove throwExceptions="true" adjustment for testing, as it was causing a test to fail because NLog was trying to create the "Octopus Tentacle" event source, and it already existed. This is not an issue normally, as throwExceptions="false" normally.

It was considered not worth investing time to fix this. So we deleted this enhancement.

@sburmanoctopus sburmanoctopus merged commit cd04022 into main Dec 14, 2023
48 checks passed
@sburmanoctopus sburmanoctopus deleted the sast/FixingShouldLogStartupDiagnosticsToInstanceLogFileOnly branch December 14, 2023 23:12
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.

3 participants