scientistproject / Scientist.net

A .NET library for carefully refactoring critical paths. It's a port of GitHub's Ruby Scientist library
MIT License
1.47k stars 95 forks source link

Added BeforeRun option for expensive setup #28

Closed jtreuting closed 8 years ago

jtreuting commented 8 years ago

Implemented the BeforeRun that is available in the Ruby version. Followed the current code for handling async. Added a couple simple unit tests at this point to make sure that the method was called if it existed (meaning, not null).

jtreuting commented 8 years ago

I haven't used NSubstitute before so maybe I'm just missing something obvious, but I'm not seeing how it will prevent the need for storing beforeRunRan since the BeforeRun Action doesn't return anything.

I've changed one test to this to use IControlCandidate<T>, what else were you thinking? Thanks.

        var beforeRunRan = false;

        // We introduce side effects for testing. Don't do this in real life please.
        var mock = Substitute.For<IControlCandidate<int>>();
        mock.Control().Returns(42);
        mock.Candidate().Returns(42);
        Action beforeRun = () => { beforeRunRan = true; };

        var result = Scientist.Science<int>("beforeRun", experiment =>
        {
            experiment.BeforeRun(beforeRun);
            experiment.Use(mock.Control);
            experiment.Try(mock.Candidate);
        });

        Assert.Equal(42, result);
        Assert.True(beforeRunRan);
joncloud commented 8 years ago

You should be able to add a new interface method onto IControlCandidate<T> to account for BeforeRun (it can return void, and take no arguments). Then when you use mock to define its behavior, you can use:

mock.Received().BeforeRun()

This will make sure that when you pass experiment.BeforeRun(mock.BeforeRun) the system makes a call into the interface (substituted by NSubstitute). If the system doesn't call into the method, then an exception will be thrown that will turn into a failed test.

haacked commented 8 years ago

Looking good so far. Just need merge master into it or rebase it against master. Sorry for the conflicts!

jtreuting commented 8 years ago

Okay, I merged the upstream master back into mine and resolved the conflicts and made the small changes needed to integrate it with the comparator logic that was added.

haacked commented 8 years ago

Thank you for your contribution!

haacked commented 8 years ago

selfie-0