Visitar URL original
Get the benchmarks running again by stevenaw · Pull Request #4897 · nunit/nunit · GitHub
Skip to content

Get the benchmarks running again - #4897

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

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

Conversation

@stevenaw

@stevenaw stevenaw commented Dec 15, 2024

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

stevenaw commented Dec 15, 2024

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

manfred-brands
manfred-brands requested changes Dec 15, 2024
View reviewed changes

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/NUnitFramework/benchmarks/nunit.framework.benchmarks/nunit.framework.benchmarks.csproj
@stevenaw

stevenaw commented Dec 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

manfred-brands commented Dec 15, 2024

Copy link
Copy Markdown
Member

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

@stevenaw

stevenaw commented Dec 15, 2024

Copy link
Copy Markdown
Member Author

Sure! Feel free, thanks @manfred-brands

manfred-brands
manfred-brands approved these changes Dec 15, 2024
View reviewed changes

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

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.

mikkelbu
mikkelbu reviewed Dec 15, 2024
View reviewed changes
Comment thread src/NUnitFramework/benchmarks/nunit.framework.benchmarks/nunit.framework.benchmarks.csproj Outdated
@stevenaw

stevenaw commented Dec 15, 2024

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

stevenaw commented Dec 15, 2024

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.

mikkelbu
mikkelbu approved these changes Dec 15, 2024
View reviewed changes

@mikkelbu mikkelbu left a comment

Copy link
Copy Markdown
Member

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

stevenaw commented Dec 15, 2024

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

Reviewers

@manfred-brands manfred-brands manfred-brands approved these changes
@mikkelbu mikkelbu mikkelbu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Benchmarks fail to run

3 participants

@stevenaw @manfred-brands @mikkelbu