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.

Sign up for GitHub

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

Jump to bottom

fix: fix unit and integ tests hanging #1954

Merged
merged 2 commits into from
Jan 30, 2025
Merged

fix: fix unit and integ tests hanging #1954

merged 2 commits into from
Jan 30, 2025

Conversation

philasmar
Copy link
Contributor

@philasmar philasmar commented Jan 29, 2025 •
edited
Loading

Issue #, if available:
DOTNET-7945

Description of changes:

  • I have updated the tests to atomically get the next available port for the processes we are spinning up.
  • Added DirectoryHelpers to create temp working directories for the tests that modify the project in place.
  • Skipped the tests that are hanging to unblock the feature branch. Will work on the hanging test as a follow up.

Note: The unit tests are now passing in the GitHub CI and Release Pipeline. However, the integration tests are still failing. Will work on that as a follow up.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@philasmar philasmar requested review from normj and a user January 29, 2025 19:59
@philasmar philasmar added the Release Not Needed Add this label if a PR does not need to be released. label Jan 29, 2025
ghost
ghost reviewed Jan 29, 2025
View reviewed changes
...stTool-v2/tests/Amazon.Lambda.TestTool.UnitTests/Processes/ApiGatewayEmulatorProcessTests.cs Outdated
@@ -21,7 +21,7 @@ public async Task RouteNotFound(ApiGatewayEmulatorMode mode, HttpStatusCode stat
// Arrange
var cancellationSource = new CancellationTokenSource();
cancellationSource.CancelAfter(5000);
var settings = new RunCommandSettings { ApiGatewayEmulatorPort = 9003, ApiGatewayEmulatorMode = mode, NoLaunchWindow = true};
var settings = new RunCommandSettings { ApiGatewayEmulatorPort = 9004, ApiGatewayEmulatorMode = mode, NoLaunchWindow = true};
Copy link

Choose a reason for hiding this comment

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

one other idea for the ports is that we can automatically find any open ports and use those. not sure if thats related to your issue or not though

@philasmar philasmar force-pushed the asmarp/fix-unit-tests branch 2 times, most recently from 6643b4e to bffb72a Compare January 30, 2025 17:01
ghost
ghost suggested changes Jan 30, 2025
View reviewed changes
buildtools/ci.buildspec.yml Show resolved Hide resolved
Tools/LambdaTestTool-v2/tests/Amazon.Lambda.TestTool.UnitTests/PackagingTests.cs Show resolved Hide resolved
Tools/LambdaTestTool-v2/tests/Amazon.Lambda.TestTool.UnitTests/PackagingTests.cs
@@ -44,8 +44,10 @@ private string GetRuntimeSupportTargetFrameworks()
process.Start();
var output = process.StandardOutput.ReadToEnd();
var error = process.StandardError.ReadToEnd();
process.WaitForExit();
process.WaitForExit(int.MaxValue);
Copy link

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The int.MaxValue is used to workaround a microsoft bug. We don't actually intend to wait that long. We use it in other repos. Sometimes the process hangs, and when we pass int.MaxValue the process stops hanging.

Tools/LambdaTestTool-v2/tests/Amazon.Lambda.TestTool.UnitTests/Helpers/TestHelpers.cs
private static int _maxLambdaRuntimePort = 6000;
private static int _maxApiGatewayPort = 9000;

public static int GetNextLambdaRuntimePort()
Copy link

Choose a reason for hiding this comment

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

can we update this to something like https://github.com/aws/aws-lambda-dotnet/pull/1955/files#diff-16dac3e7db3ba4a144e7ac3063ba31b74a53037cb85cc1a5e35410efb0d6393dR311? the reason being is depending on the developers machine/codebuild machine, the above ports may not actually be free. I think we should use TcpListener for the OS to give us free ports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of the change I'm making is so we can atomically check what the next port is that other tests are not using. I believe the TcpListener is a bit overkill as these port ranges are always free in the CI. Even on our machines, these ports are usually not used.

@philasmar philasmar requested a review from a user January 30, 2025 17:15
@philasmar philasmar force-pushed the asmarp/fix-unit-tests branch from bffb72a to a7b9cc0 Compare January 30, 2025 18:14
@philasmar philasmar changed the title fix: fix unit tests hanging fix: fix unit and integ tests hanging Jan 30, 2025
normj
normj approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

Approve but I recommend when add the skips for tests for now still allow the test to run in debug mode.

#if !DEBUG
[Fact(Skip = "Skipping this test as it is not working properly.")]
#endif

@philasmar philasmar force-pushed the asmarp/fix-unit-tests branch from 03d9a1e to d4203ee Compare January 30, 2025 19:11
@philasmar
Copy link
Contributor Author

Approve but I recommend when add the skips for tests for now still allow the test to run in debug mode.

#if !DEBUG
[Fact(Skip = "Skipping this test as it is not working properly.")]
#endif

Fixed

@philasmar philasmar merged commit 1937ca3 into feature/lambdatesttool-v2 Jan 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normj normj normj approved these changes

Assignees
No one assigned
Labels
Release Not Needed Add this label if a PR does not need to be released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@philasmar @normj