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

[Suggestion] Please keep [Setup]/[Cleanup] attributes #456

Closed
ig-sinicyn opened this Issue Jun 1, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@ig-sinicyn
Collaborator

ig-sinicyn commented Jun 1, 2017

The setup attributes were renamed to [GlobalSetup]/[GlobalCleanup] recently.

I think there is no need to introduce Global prefix, old names are fine. Most of unit test frameworks do use same names for similar attributes and therefore old names were self-explanatory. In code it looks fine, too:

[Setup]
public void Setup() { ... }

[IterationSetup]
public void SetupIteration() { ... }

As additional proposal (have no good arguments for it, though). If the Setup/Cleanup will be renamed back, the IterationSetup attribute may be renamed to SetupIteration, this way it will be easier to discover a new attribute via code completion

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Jun 1, 2017

Member

Hey @ig-sinicyn, thanks for your feedback. I want to keep [GlobalSetup]/[GlobalCleanup] because of the following reason:

  1. Our users don't understand when the setup method is called. Yeah, we covered it in the documentation, but who reads it? Especially when a developer see the benchmark code somewhere (e.g. on StackOverflow or GitHub), and he/she isn't familiar with BenchmarkDotNet, it's hard to figure out what [Setup] really means? A lot of people think that the setup method will be invoked before each benchmark target method invocation so such name can be confused. I hope that it will be obvious for people (especially for new users) that [GlobalSetup] is, you know, a global setup.
  2. While I made changes in attributes, I realized that it's much easier to do any refactorings when you can match all usages of the attribute infrastructure by name. I mean that if we keep the [Setup], [SetupIteration] names, it will be easy to find all the code related to [SetupIteration], but it will be hard to find code related to [Setup] but not [SetupIteration]. With [GlobalSetup], we don't have such problem.

@adamsitnik what do you think?

Member

AndreyAkinshin commented Jun 1, 2017

Hey @ig-sinicyn, thanks for your feedback. I want to keep [GlobalSetup]/[GlobalCleanup] because of the following reason:

  1. Our users don't understand when the setup method is called. Yeah, we covered it in the documentation, but who reads it? Especially when a developer see the benchmark code somewhere (e.g. on StackOverflow or GitHub), and he/she isn't familiar with BenchmarkDotNet, it's hard to figure out what [Setup] really means? A lot of people think that the setup method will be invoked before each benchmark target method invocation so such name can be confused. I hope that it will be obvious for people (especially for new users) that [GlobalSetup] is, you know, a global setup.
  2. While I made changes in attributes, I realized that it's much easier to do any refactorings when you can match all usages of the attribute infrastructure by name. I mean that if we keep the [Setup], [SetupIteration] names, it will be easy to find all the code related to [SetupIteration], but it will be hard to find code related to [Setup] but not [SetupIteration]. With [GlobalSetup], we don't have such problem.

@adamsitnik what do you think?

@ig-sinicyn

This comment has been minimized.

Show comment
Hide comment
@ig-sinicyn

ig-sinicyn Jun 1, 2017

Collaborator

@AndreyAkinshin well, I see your point now and it looks better than mine:)

Collaborator

ig-sinicyn commented Jun 1, 2017

@AndreyAkinshin well, I see your point now and it looks better than mine:)

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jun 1, 2017

Member

I agree, @AndreyAkinshin has very good arguments. I was asked how often is [Setup] invoked so many times that I hope that now it will change.

Member

adamsitnik commented Jun 1, 2017

I agree, @AndreyAkinshin has very good arguments. I was asked how often is [Setup] invoked so many times that I hope that now it will change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment