Skip to content

Get the benchmarks running again#4897

Merged
stevenaw merged 4 commits intomainfrom
fix-the-benchmarks
Dec 15, 2024
Merged

Get the benchmarks running again#4897
stevenaw merged 4 commits intomainfrom
fix-the-benchmarks

Conversation

@stevenaw
Copy link
Copy Markdown
Member

Fixes #4806

This PR gets the benchmarks running again by following the steps outlined in the issue. I was able to inject the assemblyconfiguration at runtime by defining a BenchmarkDotNet Job based on the default one

@stevenaw
Copy link
Copy Markdown
Member Author

This is ready for review.

However, this could be taken one step further by updating the TFM from net6.0 to something newer. Should it be updated to latest (NET 9.0) or latest LTS (net8.0)? Note that latest LTS will hit EOL later I believe

Copy link
Copy Markdown
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

I currently get compile time errors when DotNetBenchMark is compiling the code, i.e. when running.
MSBuild wants GenerateDocumentationFile be set. We have it set on all other projects, I think we should move it to Directory.Build.props instead. For some reason MS can only check for 'unused namespace' if documentation is enabled.
However setting that results in CS1591 errors as there is no documentation, we have disabled that in various other test projects.

@stevenaw
Copy link
Copy Markdown
Member Author

stevenaw commented Dec 15, 2024

Thanks @manfred-brands .
Odd, I didn't get those on the current TFM, but I had when moving to net8.0 initially.

I can make those updates tomorrow. It'll likely also require a NoWarn to suppress the warning for missing xmldocs on public members (1591 ?) similar to how our other test projects are already setup.

EDIT: Ah, you covered that already in your review comment too. Thanks!

@manfred-brands
Copy link
Copy Markdown
Member

I made them here and can push them if you are ok with that.

@stevenaw
Copy link
Copy Markdown
Member Author

Sure! Feel free, thanks @manfred-brands

Copy link
Copy Markdown
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

I'm happy with this now. Verify it runs for you.

@stevenaw
Copy link
Copy Markdown
Member Author

Thanks @manfred-brands @mikkelbu
I should be able to get to both of those in a few hours

@stevenaw
Copy link
Copy Markdown
Member Author

The requested changes are done and I've confirmed that @manfred-brands 's changes also work for me.

I'll wait a few days before merging in case anyone wants to take another look.

Copy link
Copy Markdown
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Still looks good to me :). Thanks for the work @stevenaw and @manfred-brands 👍

@stevenaw
Copy link
Copy Markdown
Member Author

Thanks @mikkelbu !

@stevenaw stevenaw merged commit b7064b6 into main Dec 15, 2024
@stevenaw stevenaw deleted the fix-the-benchmarks branch December 15, 2024 18:58
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.

Benchmarks fail to run

3 participants