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

Improve System Test performance with stubbing dotnet new #294

Merged
merged 9 commits into from
Nov 6, 2024

Conversation

gasparnagy
Copy link
Contributor

@gasparnagy gasparnagy commented Oct 15, 2024

🤔 What's changed?

The TestProjectGenerator that is used by System Tests (and currently Specs tests as well) has been improved:

  • the dotnet new sln and the dotnet new classlib commands have been replaced by a "stub" version that calls the original command first, but caches the result in the temp folder and for the next time it only copies the files
  • the dotnet sln add command has been replaced by a stub implementation that does the same by editing the .sln file directly

⚡️ What's your motivation?

The main motivation is performance as these three basic command take significant time in local and CI execution.

With the changes I have achieved a good performance improvement locally:

  • execution time of all system tests were 213 s (3:33) before
  • with the changes and empty cache: 166 s (2:46), 22% improvement
  • with the changes and populated cache: 158 s (2:38), 26% improvement

I also hope that this fixes #249 too.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

It can happen that this changed the behavior of #109, so that needs to be retested.

📋 Checklist:

  • I've changed the behaviour of the code

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

@obligaron
Copy link
Contributor

I added a fix for the Linux build (a replacement for the dictionary separator / was missing).
I also checked that #109 still worked. It was necessary to bind the cache to the sdk version. This will also help if different sdk versions produce different output in the future (e.g. different .sln format).

@gasparnagy gasparnagy marked this pull request as ready for review November 4, 2024 15:54
@gasparnagy
Copy link
Contributor Author

@obligaron super, thx! Could you please make a review of the rest of the PR?

@gasparnagy
Copy link
Contributor Author

gasparnagy commented Nov 4, 2024

Update: Ignore this, I added this command already.
@obligaron Also folks here mentioned that we could add a --no-restore option for the new classlib for better performance. Can you try that out?

@gasparnagy gasparnagy requested a review from obligaron November 5, 2024 08:59
@gasparnagy gasparnagy self-assigned this Nov 5, 2024
Copy link
Contributor

@obligaron obligaron left a comment

Choose a reason for hiding this comment

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

lgtm 🙂

@gasparnagy gasparnagy merged commit 4eed410 into main Nov 6, 2024
4 checks passed
@gasparnagy gasparnagy deleted the improve_systemtests_dotnet_new branch November 6, 2024 08:51
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.

Reqnroll CI build fail randomly
2 participants