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

Run more tests in temporary directory #17683

Merged
merged 13 commits into from
Sep 11, 2024
Merged

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Sep 10, 2024

Some tests cases were executing in this repo subfolders, often many tests reusing the same folders. This had disadvantages.
You can't run such tests cases concurrently and sometimes git changes get polluted with test artifacts.

The idea is to simply copy the directory required by the test, with subfolders, to temp.

On each test run a subfolder is created in %TEMP%/FSharp.Test.Utilities/ named with current date and a short uinque id: %TEMP%/FSharp.Test.Utilities/yyy-MM-dd-xxxxxxx/.
Test cases executed during the test run create separate uniquely named folders inside.

This PR is extracted from the experiments in test parallelization.

Copy link
Contributor

✅ No release notes required

@KevinRansom
Copy link
Member

@majocha - This looks good, is it ready for review, or will you do the Big Todo in this PR?

@majocha
Copy link
Contributor Author

majocha commented Sep 10, 2024

@KevinRansom if it's ok I'd rather not, because frankly I have no idea how to even approach it :)

@majocha majocha marked this pull request as ready for review September 10, 2024 07:40
@majocha majocha requested a review from a team as a code owner September 10, 2024 07:40
tests/fsharp/tests.fs Outdated Show resolved Hide resolved
@majocha
Copy link
Contributor Author

majocha commented Sep 10, 2024

SDKTests does not seem to produce any artifacts, so it can safely run in-place.
The test just executes msbuild so if it ever causes problems we can probably pass a property to it and just set different output path.

@psfinaki
Copy link
Member

SDKTests does not seem to produce any artifacts, so it can safely run in-place.
The test just executes msbuild so if it ever causes problems we can probably pass a property to it and just set different output path.

Yes, I have also checked this. So theoretically migrating it to the new platform shouldn't be a problem?

It uses this exec function to just run it like msbuild C:\code\fsharp2\tests\fsharp\SDKTests\AllSdkTargetsTests.proj /p:Configuration=Debug. In the CI, I cannot find the error that was showing up there, but just noting that there are some other helper functions in the TestFramework which might help, e.g. execIn with the ability to specify the dir.

@majocha
Copy link
Contributor Author

majocha commented Sep 10, 2024

Yes, I have also checked this. So theoretically migrating it to the new platform shouldn't be a problem?

It uses this exec function to just run it like msbuild C:\code\fsharp2\tests\fsharp\SDKTests\AllSdkTargetsTests.proj /p:Configuration=Debug. In the CI, I cannot find the error that was showing up there, but just noting that there are some other helper functions in the TestFramework which might help, e.g. execIn with the ability to specify the dir.

My guess is we can just modify the files to use a custom prop instead of $MSBuildThisFileDirectory and pass the correct path in code via "-property:" but it does not seem to be needed at all right now.

@psfinaki
Copy link
Member

My guess is we can just modify the files to use a custom prop instead of $MSBuildThisFileDirectory and pass the correct path in code via "-property:" but it does not seem to be needed at all right now.

Yeah, that's my feeling as well. Shall we once again try to remove testConfigOldBehavior to verify that? If you're tired of this and want to move on, feel free to say no - we can merge this as it is as well, since otherwise things look good to me and the PR vastly clarifies and improves the infra anyway.

@majocha
Copy link
Contributor Author

majocha commented Sep 10, 2024

Let's try it for completness.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Alrighty, so that's a progress, let's get this in! Thanks @majocha :)

(just in case, I checked the number of tests being run in the PR, all good :D)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants