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

Attributes put on base methods are not considered in derived class #495

Closed
smitpatel opened this Issue Jul 18, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@smitpatel
Contributor

smitpatel commented Jul 18, 2017

class Program
{
    static void Main(string[] args)
    {
        BenchmarkRunner.Run<Derived>();
    }
}

public abstract class Base
{
    protected int SleepTime;

    [IterationSetup]
    public abstract void Setup();
        
    [Benchmark]
    public void Test()
    {
        Thread.Sleep(SleepTime);
    }
}

public class Derived : Base
{
    //[IterationSetup]
    public override void Setup()
    {
        SleepTime = 100;
    }
}

In above code I have annotated method Base.Setup with IterationSetup. When I override the method to provide implementation in derived class but don't put attribute (assuming it will come from base class automatically), then the method is not called. This makes SleepTime = default(int) which is 0.
If I put attribute in derived class then it sets proper value.
Without attribute in derived class

Method |     Mean |    Error |   StdDev |
------- |---------:|---------:|---------:|
   Test | 164.9 ns | 3.194 ns | 3.802 ns |

With attribute in derived class

 Method |     Mean |     Error |    StdDev |
------- |---------:|----------:|----------:|
   Test | 100.5 ms | 0.0955 ms | 0.0893 ms |

The framework should consider attributes put on base methods.

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jul 25, 2017

Member

hi @smitpatel !

Thanks for a good report. I think that you are right, the expected behavior is different than what we have today.

@AndreyAkinshin do you agree? It should be easy to implement

Member

adamsitnik commented Jul 25, 2017

hi @smitpatel !

Thanks for a good report. I think that you are right, the expected behavior is different than what we have today.

@AndreyAkinshin do you agree? It should be easy to implement

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin
Member

AndreyAkinshin commented Jul 25, 2017

@adamsitnik, LGTM.

@lukasz-pyrzyk

This comment has been minimized.

Show comment
Hide comment
@lukasz-pyrzyk

lukasz-pyrzyk Aug 2, 2017

Collaborator

@AndreyAkinshin can you assign it to me?

I'm working on it. Thinking of a suitable unit test.

Collaborator

lukasz-pyrzyk commented Aug 2, 2017

@AndreyAkinshin can you assign it to me?

I'm working on it. Thinking of a suitable unit test.

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 2, 2017

Member

@lukasz-pyrzyk, assigned.

A short note about unit tests: usually we check such stuff via integration tests which is wrong (we do because it's the easiest way). The main problem here is that it significantly increases the total time of the whole test suite (especially for .NET Core profile).
In fact, we should introduce tests which build the benchmark sets via our API (based on the class description) and check it manually (instead of parsing logs in integration tests).

Member

AndreyAkinshin commented Aug 2, 2017

@lukasz-pyrzyk, assigned.

A short note about unit tests: usually we check such stuff via integration tests which is wrong (we do because it's the easiest way). The main problem here is that it significantly increases the total time of the whole test suite (especially for .NET Core profile).
In fact, we should introduce tests which build the benchmark sets via our API (based on the class description) and check it manually (instead of parsing logs in integration tests).

@adamsitnik adamsitnik closed this in 74b41ef Aug 2, 2017

@AndreyAkinshin AndreyAkinshin added this to the v0.10.10 milestone Aug 2, 2017

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018

Unit test for reading attributes from the base class, fixes dotnet#495
* dotnet#495 - Unit test for reading attributes from the base class

* Assertion for method declaring type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment