Ignore #47

Merged
merged 8 commits into from Mar 11, 2016

Projects

None yet

5 participants

@davezych
Contributor
davezych commented Mar 1, 2016

Allow ignoring of results. User can enter 1 or more checks to ignore a mismatch. If a mismatch is ignored:

  • The result of the experiment is Matched
  • MismatchedObservations is empty
  • IgnoredObservations is populated
Dave Zych added some commits Mar 1, 2016
@joncloud joncloud and 1 other commented on an outdated diff Mar 2, 2016
src/Scientist/IExperiment.cs
@@ -36,6 +36,12 @@ public interface IExperiment<T>
/// Defines a custom func used to compare results.
/// </summary>
void Compare(Func<T, T, bool> comparison);
+
+ /// <summary>
+ /// Defines the check to run to determine if mismatches should be ignored.
+ /// </summary>
+ /// <param name="block">The delegate to execute</param>
+ void Ignore(Func<bool> block);
@joncloud
joncloud Mar 2, 2016 Contributor

Shouldn't Ignore take a Func<Observation<T>, Observation<T>, bool> or Func<T, T, bool>? It looks like the ruby framework will submit a call that passes the control and candidates values in, and returns back the boolean result.

https://github.com/github/scientist/blob/master/lib/scientist/experiment.rb#L153

@davezych
davezych Mar 2, 2016 Contributor

@joncloud yeah, I think you're right. I'll update.

@davezych
Contributor
davezych commented Mar 2, 2016

@joncloud I've updated the ignore funcs to include the observation results.

One thing I'm not understanding is in the ruby doc, it says

The ignore blocks are only called if the values don't match. If one observation raises an exception and the other doesn't, it's always considered a mismatch. If both observations raise different exceptions, that is also considered a mismatch.

but I'm not seeing that in the code anywhere. I can see that it checks equivalence based on values and exceptions raised (here: https://github.com/github/scientist/blob/master/lib/scientist/observation.rb#L61-L83), however when checking ignored it looks like it ignores regardless of exception status (here: https://github.com/github/scientist/blob/master/lib/scientist/result.rb#L66-L76)

Am I misunderstanding the docs, or the code?

@joncloud
Contributor
joncloud commented Mar 2, 2016

I'm not a ruby expert by any means, but it seems like it should be handling it according to the documentation. What do you think about passing in the experiment into the result constructor similar to the ruby framework and using something similar to the following lines?

IgnoredObservations = MismatchedObservations.Where(o => experiment.IgnoresMismatchedObservation(control, o)).ToList();
MismatchedObservations = MismatchedObservations.Except(IgnoredObservations).ToList();
@joncloud
Contributor
joncloud commented Mar 2, 2016

Actually I just realized that IgnoresMismatchedObservations returns Task<bool>, so if that is the case this would have to be calculated before the constructor call, or the constructor would have to move into a static method returning Task<Result<T>>.

@davezych
Contributor
davezych commented Mar 2, 2016

@joncloud

What do you think about passing in the experiment into the result constructor similar to the ruby framework

that's actually what I updated it to do 👍

I guess I'm not understanding this code, because to me it seems that if candidate throws and control doesn't (for instance), the observation is included in mismatch which is correct, but then it will potentially get ignored by the ignore blocks regardless of exception status. This seems to go against the wording in the docs.

Perhaps @Haacked can clear up my confusion?

@Haacked
Member
Haacked commented Mar 3, 2016

I guess I'm not understanding this code, because to me it seems that if candidate throws and control doesn't (for instance), the observation is included in mismatch which is correct, but then it will potentially get ignored by the ignore blocks regardless of exception status. This seems to go against the wording in the docs.

I'm not a Ruby expert, but looking at how Scientist determines whether it's ignored:
https://github.com/github/scientist/blob/5fea1c6a2a97f0adc743566fe85ba885cd18e1c3/lib/scientist/experiment.rb#L149-L159

It looks like it returns false if either throws an exception. Thus it's a mismatch and not ignored. Which matches what the docs say here:

The ignore blocks are only called if the values don't match. If one observation raises an exception and the other doesn't, it's always considered a mismatch. If both observations raise different exceptions, that is also considered a mismatch.

Does that clear it up or am I missing something?

@davezych
Contributor
davezych commented Mar 3, 2016

Hmm... I'm still missing something.

Let's say we run an experiment, and control runs successfully but candidate throws an exception. In the ruby lib, it runs evaluate_candidates which checks if the observations are equal. In this case one threw and the other didn't, so they are mismatched. The mismatched var now includes the candidate.

We then iterate through mismatched and run ignore_mismatched_observation for each observation. It loops through each ignore block and returns the result. If any of them return true, the mismatch is ignored. As you pointed out, it returns false if the ignore block throws.

I'm not seeing where it will ignore the ignore block if the original candidate observation threw. In my mind, if our ignore block returns true (for instance if our ignore block is to check if today is Monday), then the observation is ignored regardless of whether the original observation threw.

This seems to go against the docs, which I'm reading as "if one threw and the other didn't, the observation is never ignored and it's always a mismatch."

Maybe I'm reading into this too deeply.

@Haacked
Member
Haacked commented Mar 3, 2016

Hmm, hey @jesseplusplus, do you have time to help us grok the expected behavior of the Ruby library?

The question starts in the comment #47 (comment)

@davezych
Contributor

Hey @jbarnette think you can help us grok the expected behavior of the ignore feature of Scientist? It looks like you wrote the original code.

The question/confusion starts on this comment

@jbarnette
Member

@Haacked @davezych Jesse and I are both pretty swamped today, but at least one of us will catch up on this thread shortly. Might be early next week though.

@jbarnette
Member

/snipe @zerowidth

@Haacked
Member
Haacked commented Mar 11, 2016

Might be early next week though.

🤘 It's not a rush. We'll check back in with you in a couple weeks if we don't hear from you. 😄

@jesseplusplus
Member

Sorry that the docs are a bit confusing here.

I'm not seeing where it will ignore the ignore block if the original candidate observation threw. In my mind, if our ignore block returns true (for instance if our ignore block is to check if today is Monday), then the observation is ignored regardless of whether the original observation threw.
This seems to go against the docs, which I'm reading as "if one threw and the other didn't, the observation is never ignored and it's always a mismatch."

The docs are saying that only mismatches will potentially be ignored. The case that you are giving where the candidate throws an exception but the control does not is considered a mismatch, so it will run through the ignore code and potentially be ignored if the ignore block returns true.

The ruby code currently only passes the value and not the exception to the ignore block, which probably isn't ideal since the values of observations that raise are just null. If you wanted smarter ignores that could do something based on the exception raised, you would have to change the ignore API to take in the full candidate and control observations rather than just the values.

@davezych
Contributor

Ah, makes sense!

Thanks @jesseplusplus @jbarnette!

Dave Zych Spacing 0d0d99c
@davezych
Contributor

@Haacked based on Jesse's comment above this should be good to go.

@Haacked Haacked merged commit 1b40bdc into github:master Mar 11, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Haacked
Member
Haacked commented Mar 11, 2016

@davezych davezych deleted the davezych:Ignore branch Mar 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment