-
Notifications
You must be signed in to change notification settings - Fork 744
Add comprehensive test framework for TraceParserGen #2308
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: brianrob <[email protected]>
Co-authored-by: brianrob <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, please address these comments.
// Since we don't have a real source, we'll just verify the type can be instantiated | ||
// by checking if it has expected methods | ||
|
||
var enumerateMethod = parserType.GetMethod(""EnumerateTemplates"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of searching for EnumerateTemplates
lets assume that the caller of this executable passed in the path to a trace file with events that will be parsed by the generated parser. You can create a TraceEventSource by calling TraceEventDispatcher.GetDispatcherFromFileName. Then you can make sure that the events show up properly in the parser by hooking the TraceEventParser.All on the generated parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test application to use TraceEventDispatcher.GetDispatcherFromFileName
with a trace file. The generated parser is now instantiated with the source, hooks TraceEventParser.All
to count events, and processes the trace to validate the parser works correctly. Commit: f5dd42c
var startInfo = new ProcessStartInfo | ||
{ | ||
FileName = "dotnet", | ||
Arguments = "run -c Release --no-build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove --no-build
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed --no-build
flag. Commit: f5dd42c
src/TraceParserGen.Tests/TestBase.cs
Outdated
string[] searchPaths = new[] | ||
{ | ||
// Relative to test output directory | ||
Path.Combine(Environment.CurrentDirectory, @"../../../../TraceParserGen/bin/Release/net462/TraceParserGen.exe"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a deterministic way to get TraceParserGen.exe. Instead, please update the TraceParserGen.Tests.csproj to depend on the TraceParserGen build. Copy the binary that matches the flavor (debug/release) into the binaries directory when building TraceParserGen.Tests. Then, you can directly point at the current bin directory for the exe to target rather than searching for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ProjectReference
to TraceParserGen and a build target that copies TraceParserGen.exe to the test output directory. GetTraceParserGenExePath()
now simply returns the exe from the current directory. Commit: f5dd42c
src/TraceParserGen.Tests/TestBase.cs
Outdated
protected string GetTraceEventAssemblyPath() | ||
{ | ||
// Search for TraceEvent DLL in common build output locations | ||
string[] searchPaths = new[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than referencing TraceEvent.dll directly, please reference the Project via a in the test applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test project already references TraceEvent via ProjectReference
. The generated test app uses the TraceEvent.dll from the test project's output directory (where it's copied as part of the ProjectReference). Commit: f5dd42c
CreateNoWindow = true | ||
}; | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests only run on Windows, so you can remove the else here and just unconditionally run the Windows path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the else block and mono-related code. Tests now only support Windows execution path. Commit: f5dd42c
Co-authored-by: brianrob <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address this comment.
}} | ||
}} | ||
}} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else path. We always want to create a parser and process the trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the else path. The test app now always creates a parser and processes the trace file. If the trace file doesn't exist, it returns an error instead of skipping. Commit: 04562ba
Co-authored-by: brianrob <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address these comments.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>net462;net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests themselves don't need to run against multiple TFMs. Please just run these against net462. Then you can also remove the code that confirms that we're only running on Windows, since net462 can only run on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to target net462 only and removed Windows platform check. Commit: 5f3a8c7
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\TraceEvent\TraceEvent.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the TraceEvent ProjectReference - it is not needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed TraceEvent ProjectReference. Commit: 5f3a8c7
src/TraceParserGen.Tests/TestBase.cs
Outdated
string dir = Environment.CurrentDirectory; | ||
while (dir != null) | ||
{ | ||
string candidate = Path.Combine(dir, @"TraceParserGen.Tests\inputs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should just be "inputs" because the inputs are being copied into the output directory and the output directory will be the current directory when running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed TestDataDir to just "inputs"
since inputs are copied to the output directory. Commit: 5f3a8c7
src/TraceParserGen.Tests/TestBase.cs
Outdated
{ | ||
protected static string OriginalInputDir = FindInputDir(); | ||
protected static string TestDataDir = Path.GetFullPath("inputs"); | ||
protected static string BaseOutputDir = Path.GetFullPath("output"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using BaseOutputDir, just use .NET's standard facility to get a random directory in TEMP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use Path.GetTempPath()
instead of custom BaseOutputDir. Commit: 5f3a8c7
src/TraceParserGen.Tests/TestBase.cs
Outdated
OutputDir = Path.Combine(BaseOutputDir, Guid.NewGuid().ToString("N").Substring(0, 8)); | ||
|
||
// Ensure output directory exists | ||
if (!Directory.Exists(OutputDir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to check - Directory.CreateDirectory will check before attempting to create it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the existence check - Directory.CreateDirectory
handles it. Commit: 5f3a8c7
…Event reference Co-authored-by: brianrob <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address these comments.
src/TraceParserGen.Tests/TestBase.cs
Outdated
/// <summary> | ||
/// Finds the input directory for test files | ||
/// </summary> | ||
private static string FindInputDir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not needed - since you're operating from Environment.CurrentDirectory, the location of the inputs should be Path.Combine(Environment.CurrentDirectory, "inputs")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed FindInputDir method. TestDataDir now uses Path.Combine(Environment.CurrentDirectory, "inputs")
directly. Commit: 0bc8f45
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Reference Include=""Microsoft.Diagnostics.Tracing.TraceEvent""> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using a <Reference>
tag, this should be a <ProjectReference>
tag pointing at TraceEvent.csproj in the repo. You'll need to implement this by using a relative path (e.g. ....\TraceEvent\TraceEvent.csproj). This might not be the right relative path, but you can tell by finding the source code at runtime. You'll need to do this because TraceEvent.dll won't be the only dependency needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use <ProjectReference>
tag pointing to TraceEvent.csproj. The path is found at runtime by navigating from the test assembly location to the source directory. Commit: 0bc8f45
Co-authored-by: brianrob <[email protected]>
Summary
This PR introduces a new test project
TraceParserGen.Tests
that implements a comprehensive test framework for validating TraceParserGen.exe.Changes Made
Latest Updates
Path.Combine(Environment.CurrentDirectory, "inputs")
directly since inputs are copied to the output directory<Reference>
tag to<ProjectReference>
pointing to TraceEvent.csproj, ensuring all dependencies are includedPrevious Updates
Path.GetTempPath()
Directory.CreateDirectory
TraceEventDispatcher.GetDispatcherFromFileName
and hooksTraceEventParser.All
--no-build
flag - Test app rebuilds each timeTest Framework Features
The test framework validates the entire code generation pipeline:
All
event to count processed eventsBenefits
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.