Skip to content

Conversation

@Forgind
Copy link
Contributor

@Forgind Forgind commented May 2, 2022

Progress on #7377

Context

Adds a test for MSBuild server. This covers

  • ensuring the server starts when requested,
  • ensuring the requesting node dies thereafter,
  • ensuring the server persists for future builds,
  • ensuring that the server node can be killed even in the middle of a build without preventing the next build from succeeding,
  • ensuring that that next build also uses the MSBuild server node but it has a different process ID from the previous MSBuild server node.

Changes Made

Added one test and modified RunnerUtilities.ExecMSBuild to support it.

Testing

Ran the test, and it passed. Observed intermediate stages to ensure they were as expected.

Notes

Other tests to add:
Verify legacy behavior
Verify proper mixed behavior (legacy build after MSBuild server started)
Check behavior when starting a new build while the previous build is still executing.

@Forgind Forgind force-pushed the server-node-test branch from dbfbcbd to 96940db Compare May 3, 2022 15:54
Copy link
Member

Choose a reason for hiding this comment

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

I think 3s it way too long. I would start with 1s, put comment there "if flaky on 'Server already running' enlarge this time".
The things is that we shall, strategically, try to have fast unit tests.

@Forgind Forgind force-pushed the server-node-test branch from 6a7f4a0 to 2321e53 Compare May 12, 2022 00:53

Console.WriteLine("==== OUTPUT ====");
Console.WriteLine(output);
Console.WriteLine("Process ID is " + pid + "\r\n");
Copy link
Member

Choose a reason for hiding this comment

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

I'm likely not understanding the test, but process IDs get reused, and the algorithm used is not stable, so if a server process exits the next one can potentially get the same id?

Copy link
Member

@AR-May AR-May Jun 6, 2022

Choose a reason for hiding this comment

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

The tests should check the logic of msbuild server. That is:

  • If the special flag is on, we use msbuild server for task execution. Msbuild server process is a long living one. It lives the same time as the msbuild nodes (15 min).
  • If server is occupied, there should be a fallback to the usual, non-server, build.

The task itself writes down the id of the process that is used for the build, and here we write down the process id of the msbuild process that was created for building a project. If msbuild server was used, these ids would be different (first one would be the id of msbuild server and the second one would be the id of msbuild client). If not used, they would be the same. Thus, we check how the build was executed, in or out the server.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so long as the first process has not exited, then yes of course they have different PID's. Thanks. Otherwise of course you have to check process start time.

@rokonec rokonec merged commit 206f918 into dotnet:feature/msbuild-server Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants