davezych / shience Goto Github PK

Commit 4992e8c rolled back the runtime to rc1-update. The dependencies in both the Shience and Test projects are still targeting rc2. We should either move back up to rc2 runtime, or rollback the dependencies in each lib to latest rc1, or, potentially, move up to nightlies and move to the dotnet cli. (this would allow starting to target net platform standard and start on #15).

This all depends on if we really want to stick with stable versions. I'm okay with moving forward, however if it means we have more build/dependency restore issues of course that's not okay.

/cc @MovGP0

When .Test() is called multiple times, it overrides the Control and Candidate actions.

In such cases the user should create a new Science using the factory method Shience.New instead. Therefore, I'd prefer to have to throw an InvalidOperationException() when .Test() is called multiple times.

There should be extension methods to make use of the ExperimentResult object more fluent:

science
    .Execute()
    .OnSuccess((controlResult, candidateResult, context) => ...)
    .OnError((controlResult, candidateResult, context) => ...)

similar to #14, the source is once again not building on my machine.

When I execute

dnu restore ./src/shience

Then I get the error message

Unable to resolve dependency Microsoft.CSharp 4.0.1-beta-23516

C:\Users\Johann\Documents\shience\src\shience\project.json(14,29): error NU1001: The dependency Microsoft.CSharp >= 4.0.1-beta-23516 could not be resolved.

Version Info

I am currently using the dnu (using where dnu) from

C:\Users\Johann\.dnx\runtimes\dnx-clr-win-x64.1.0.0-rc2-16357\bin\dnu.cmd

Currently, the following .NET runtimes are installed on my machine (using dnvm list):

Active Version           Runtime Architecture OperatingSystem Alias
------ -------           ------- ------------ --------------- -----
       1.0.0-beta5       clr     x64          win
       1.0.0-beta5       clr     x86          win
       1.0.0-beta5       coreclr x64          win
       1.0.0-beta5       coreclr x86          win
       1.0.0-rc1-update1 clr     x64          win
       1.0.0-rc1-update1 clr     x86          win
       1.0.0-rc1-update1 coreclr x64          win
       1.0.0-rc1-update1 coreclr x86          win
  *    1.0.0-rc2-16357   clr     x64          win             default
       1.0.0-rc2-16357   clr     x86          win
       1.0.0-rc2-16357   coreclr x64          win
       1.0.0-rc2-16357   coreclr x86          win

Implement a runif functionality, which will allow specifying a predicate that determines whether or not to actually run the experiment. Example usages:

• If user is part of development group, run
• If user's age is < 25, run
• If user is performing action at 11:59PM, run

If a candidate test requires setup that shouldn't be included in runtime calculations, there needs to be a way to specify setup code that will run only if the candidate is going to be run.

var controlParameter = GetParam();
var candidateParameter = GetExpensiveCandidateParam();

science.Test(() => SomeSimpleCall(), () => SomeMoreComplexCall(candidateParameter))
            .Execute();

In this instance, the expensive call will always be run, which doesn't make sense if the experiment is enabled for only a fraction of requests. Something like this would be better:

var controlParameter = GetParam();
object candidateParameter = null; //Obviously object is a terrible type

science.Test(() => SomeSimpleCall(), () => SomeMoreComplexCall(candidateParameter))
            .BeforeCandidateRun(() => candidateParameter = GetExpensiveCandidateParam()) //Assuming closure rules allow this
            .Execute();

I don't believe this is needed for the control test, because that should always be run, regardless of experiment enabled percentage.

Allow user to set whether mismatch should throw an exception. Helpful probably only in unit tests.

similar to #14, the source is once again not building on my machine.

When I execute

dnu restore ./src/shience

Then I get the error message

Unable to resolve dependency Microsoft.CSharp 4.0.1-beta-23516

C:\Users\Johann\Documents\shience\src\shience\project.json(14,29): error NU1001: The dependency Microsoft.CSharp >= 4.0.1-beta-23516 could not be resolved.

Version Info

I am currently using the dnu (using where dnu) from

C:\Users\Johann\.dnx\runtimes\dnx-clr-win-x64.1.0.0-rc2-16357\bin\dnu.cmd

Currently, the following .NET runtimes are installed on my machine (using dnvm list):

Active Version           Runtime Architecture OperatingSystem Alias
------ -------           ------- ------------ --------------- -----
       1.0.0-beta5       clr     x64          win
       1.0.0-beta5       clr     x86          win
       1.0.0-beta5       coreclr x64          win
       1.0.0-beta5       coreclr x86          win
       1.0.0-rc1-update1 clr     x64          win
       1.0.0-rc1-update1 clr     x86          win
       1.0.0-rc1-update1 coreclr x64          win
       1.0.0-rc1-update1 coreclr x86          win
  *    1.0.0-rc2-16357   clr     x64          win             default
       1.0.0-rc2-16357   clr     x86          win
       1.0.0-rc2-16357   coreclr x86          win

It should be possible to chain multiple .Where() clauses on a single Science object. To do so, the Skip property should be refactored to an IList<Func<bool>> rather than an bool:

internal IList<Func<bool>> Skip { get; } = new List<Func<bool>>();

public static Science<TResult> Where<TResult>(this Science<TResult> science, Func<bool> predicate)
{
    science.Skip.Add(predicate);
    return science;
}

The predicate should not be executed immediately, but at the time the Science object gets executed. The .Execute() and .ExecuteAsync() methods should check all conditions and break using

if(science.Skip.Any(reason => reason())) return;

As mentioned in #10, IPublisher and the SetPublisher method aren't needed anymore. We will only support setting a publisher per-test, when calling New:

var science = Shience.New<bool>("test-name", (e) = { /*Do something with ExperimentResults } */);

Instead of targeting both dnxcore and dnx451, we can probably target solely netstandard1.0, which will allow targeting dnx, .net full, windows phone, etc etc etc.

The netstandard lookup table is here: https://github.com/dotnet/corefx/blob/master/Documentation/architecture/net-platform-standard.md#mapping-the-net-platform-standard-to-platforms

Scientist has the ability to ignore mismatches based on user supplied criteria.

It might be useful to provide benchmarking methods to Shience. This would allow the user to wrap Shience around a single method, and publish the runtime stats just to track the average runtime over time for a method.

A simple solution would be to allow the candidate to be null when calling Test, which would mean only the control would run and there would be publish data for just the control available.

A "better" solution would be something like:

experiment.Benchmark(() => SomeCall())
                  .PublishTo((e) => Results(e))
                  .Execute();

Phil Haack and I have been in discussions of merging projects. He started a .net port of scientist as well called Scientist.Net: https://github.com/Haacked/Scientist.net/

His plan is to move it to the Github organization sometime soon so it will be the official scientist port.

I don't really see a reason to maintain 2 separate ports of this lib and, although I like the progress we've made here and really would hate to deprecate this, think it makes sense to migrate our efforts to the "official" port.

@MovGP0 what do you think?

Right now the contexts param of the test method is a params object[]. Should it use dynamic?

The Shience/Science/Shience namespace naming is all confusing. I propose:

• Shience namespace stays
• Shience -> Science
• Science -> Experiment

It also makes more sense. When instantiating a new test, you are essentially creating an experiment, the type should reflect that.

i just did some unit tests and noted a problematic behavior:

because publisher is global, switching the publisher is problematic in multi-threaded application where multiple publishers are required. instead, the user should provide the publisher as needed via dependency injection.

As a personal opinion, there should be a fluent interface like the StringBuilder and many modern libraries like Polly have.

Shience.SetPublisher(new FilePublisher(@"C:\file\path\to\results.txt"));

var userCanRead = Shience.New<bool>("widget-permissions")
    .Setup(() => ...)
    .Test(
        control: (() => return UserPermissions.CheckUser(currentUser); ), 
        candidate: (() => return User.Can(currentUser, Permission.Read); )
    )
    .WithComparer(new PermissionComparer())
    .When(() => if-condition)
    .WithContext(new { ... })
    .TestPercentage(0.3d)
    .PublishTo(result => ...)
    .Execute(); 

if(userCanRead)
{
    //do things!
}

The advantage of this is that it becomes way easier to extend and maintain the interface. Also, the code for setting up an Scientist (extensions) can be separated from the Scientist (state) into different classes.

The user profits because the order of the properties becomes less important and the improved Auto Complete experience.

Shience will always run the candidate and control every time. Implement functionality to allow control to always run, and candidate to be run X% of the time.

Allow specifying a global percentage, and allow overwriting that percentage per test.

in order to preserve the stacktrace when the control action throws an exception, the exception should be captured using the ExceptionDispatchInfo in the ExperimentResult as an internal field and then rethrown by the .Execute() and .ExecuteAsync() methods.

In some cases the control and candidate methods may not return the same types. In our all-to-overused permissions example, let's pretend UserPermissions.CheckUser(currentUser) returns a bool, but User.Can(currentUser, Permission.Read) returns a more complex object. In that case, we would have to set up the candidate as

() => {
    var userCanRead = User.Can(currentUser, Permission.Read);
    return userCanRead.HasPermission;
}

(I realize this is a terrible example.) Although not a huge deal, it's more verbose than it needs to be. Users might also want to compare 2 different complex types which is difficult in the current setup.

I propose an interface similar to this:

public sealed class Experiment<TResult, KResult>

This allows the control and candidate to have different types if necessary. We can also overload New to allow only specifying a single parameter if both are the same:

public static Experiment<TResult, TResult> New<TResult>([NotNull]string name)
public static Experiment<TResult, KResult> New<TResult, KResult>([NotNull]string name)

In the case of specifying 2 different types the user will have to include a comparer, but I think the functionality outweighs that very slight negative.

We should think about when and how to properly test write operations.

Ie. when there are two different databases (one production and one test) and the write goes to different databases, then the comparer needs to check if the result in both databases is identical.

There could be extension methods that support this scenario by also providing (async) read operations:

science
    .TestWrite(
        control: () => OldClass.WriteToOldDatabase(importantInformation), // returns void
        candidate: () => NewClass.WriteToNewDatabase(importantInformation) // returns void
    ).Test(
        control: () => OldClass.ReadFromOldDatabase(importantInformation),
        candidate: () => NewClass.ReadFromNewDatabase(importantInformation)
    );

Async version:

await science
    .TestWriteAsync(
        control: () => OldClass.WriteToOldDatabaseAsync(importantInformation), // returns void
        candidate: () => NewClass.WriteToNewDatabaseAsync(importantInformation) // returns void
    ).TestAsync(
        control: () => OldClass.ReadFromOldDatabaseAsync(importantInformation),
        candidate: () => NewClass.ReadFromNewDatabaseAsync(importantInformation)
    );

Shience.Tests downloads Shience from NuGet instead of targeting it directly. this way, changes cannot be tested immediately and the [assembly:InternalsVisibleTo("...")] attribute gets ignored.

The publisher should not be a Functor (= class with single method; very old school), but a function (modern functional pattern).

Instead of:

Shience.Shience.SetPublisher(new MyPublisher());

The user should write something like:

Shience.Shience.SetPublisher(result => Log.Warn(...));
Shience.Shience.SetPublisher(MyPublisher.Publish);

The interface IPublisher would become obsolete.

There is no start time recorded on the experiment result, which would be very useful when publishing.

Implement publisher for databases.

Things to consider:

• How to handle creation of database tables
• Database migration/upgrades when Shience is updated
• What databases to support? Sql Server, Mysql, oracle, postgre?