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

[bug] Bug in GetTargetedMatchingMethod() logic #511

Closed
ig-sinicyn opened this Issue Jul 30, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@ig-sinicyn
Collaborator

ig-sinicyn commented Jul 30, 2017

BDN 0.10.9.

The BenchmarkConverter.GetTargetedMatchingMethod() method depends on order of methods passed to it, first match wins.

The issue is, there is no warranty that setup/cleanup method with matching Target will be enumerated before setup/cleanup method with empty target. As a proof:

		[GlobalSetup]
		public static void SetupAlwaysWins() { ... }

		[GlobalSetup(Target = nameof(MyMethod))]
		public static void SetupNeverCalled() { ... }
@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin
Member

AndreyAkinshin commented Jul 30, 2017

@ipjohnson

This comment has been minimized.

Show comment
Hide comment
@ipjohnson

ipjohnson Jul 30, 2017

Contributor

I hadn't gone down the road of Targeted and non Targeted in the same benchmark.

It seems like there are two ways to handle this

  1. throw an exception as not supported (go one way or the other)
  2. treat it as a fallback like what @ig-sinicyn filled a bug on.

Let me know which behavior is preferred and I can put in a pr in the coming days

Contributor

ipjohnson commented Jul 30, 2017

I hadn't gone down the road of Targeted and non Targeted in the same benchmark.

It seems like there are two ways to handle this

  1. throw an exception as not supported (go one way or the other)
  2. treat it as a fallback like what @ig-sinicyn filled a bug on.

Let me know which behavior is preferred and I can put in a pr in the coming days

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Jul 31, 2017

Member

I think that benchmarks which have Target and NonTarget setups at the same time look confusing. So, I suggest to show error about invalid configuration and don't run any of benchmarks.
@ig-sinicyn, @adamsitnik, what do you think?

Member

AndreyAkinshin commented Jul 31, 2017

I think that benchmarks which have Target and NonTarget setups at the same time look confusing. So, I suggest to show error about invalid configuration and don't run any of benchmarks.
@ig-sinicyn, @adamsitnik, what do you think?

@ig-sinicyn

This comment has been minimized.

Show comment
Hide comment
@ig-sinicyn

ig-sinicyn Jul 31, 2017

Collaborator

I'd prefer to follow standard approach: most specific setup wins.
This way there will be no need to include 10 setup/cleanup methods if there are 10 benchmark methods, 9 methods reuse same logic and only one needs special handling.

UPD
Also, there may be more than one targeted attribute applied to the setup/cleanup method.
Let's say, a pair of [GlobalSetup]/[GlobalCleanup] or [GlobalSetup]/[SomeDerivedGlobalSetup] attributes. Do we need to check for these weird combinations too?

Collaborator

ig-sinicyn commented Jul 31, 2017

I'd prefer to follow standard approach: most specific setup wins.
This way there will be no need to include 10 setup/cleanup methods if there are 10 benchmark methods, 9 methods reuse same logic and only one needs special handling.

UPD
Also, there may be more than one targeted attribute applied to the setup/cleanup method.
Let's say, a pair of [GlobalSetup]/[GlobalCleanup] or [GlobalSetup]/[SomeDerivedGlobalSetup] attributes. Do we need to check for these weird combinations too?

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Jul 31, 2017

Member

This way there will be no need to include 10 setup/cleanup methods if there are 10 benchmark methods, 9 methods reuse same logic and only one needs special handling.

Ok, makes sense.

Also, there may be more than one targeted attribute applied to the setup/cleanup method.

It looks too strange.

Member

AndreyAkinshin commented Jul 31, 2017

This way there will be no need to include 10 setup/cleanup methods if there are 10 benchmark methods, 9 methods reuse same logic and only one needs special handling.

Ok, makes sense.

Also, there may be more than one targeted attribute applied to the setup/cleanup method.

It looks too strange.

@ipjohnson

This comment has been minimized.

Show comment
Hide comment
@ipjohnson

ipjohnson Jul 31, 2017

Contributor

@AndreyAkinshin to summarize I'll put together a PR that allows a targeted setup/cleanup to override a non targeted.

To make things more deterministic I'll add code to throw an exception if there are to many setup/cleanup methods found.

Contributor

ipjohnson commented Jul 31, 2017

@AndreyAkinshin to summarize I'll put together a PR that allows a targeted setup/cleanup to override a non targeted.

To make things more deterministic I'll add code to throw an exception if there are to many setup/cleanup methods found.

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jul 31, 2017

Member

I believe that:

  1. More than one targeted setup/cleanup matches the benchmark = fail with error
  2. One targeted and one general setup/cleanup matches the benchmark = choose the targeted

@ipjohnson we have thing called validators that you can reuse. They are being run before benchmark execution and if you return a critical validation error no benchmark is being executed. Example

Member

adamsitnik commented Jul 31, 2017

I believe that:

  1. More than one targeted setup/cleanup matches the benchmark = fail with error
  2. One targeted and one general setup/cleanup matches the benchmark = choose the targeted

@ipjohnson we have thing called validators that you can reuse. They are being run before benchmark execution and if you return a critical validation error no benchmark is being executed. Example

@ipjohnson

This comment has been minimized.

Show comment
Hide comment
@ipjohnson

ipjohnson Jul 31, 2017

Contributor

@adamsitnik I should create a new validator, something like SetupCleanupValidator apply the logic you just outlined. Should this validator be added to DefaultConfig and CompositeValidator?

Contributor

ipjohnson commented Jul 31, 2017

@adamsitnik I should create a new validator, something like SetupCleanupValidator apply the logic you just outlined. Should this validator be added to DefaultConfig and CompositeValidator?

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jul 31, 2017

Member

@ipjohnson exactly. Thanks for doing this!!

Member

adamsitnik commented Jul 31, 2017

@ipjohnson exactly. Thanks for doing this!!

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Sep 2, 2017

Member

fixed by #525

Member

adamsitnik commented Sep 2, 2017

fixed by #525

@adamsitnik adamsitnik closed this Sep 2, 2017

@AndreyAkinshin AndreyAkinshin added this to the v0.10.10 milestone Oct 29, 2017

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