Re-enable the default rule count test case#200
Conversation
The problem was this: ValidateCustomExtension test calls ValidationRuleSet.DefaultRuleSet and makes changes to it directly. Since the DefaultRuleSet is cached, this means the private _defaultRuleSet is actually modified. The reason we are not seeing this repro locally might be that the order of test running locally and the order on AppVeyor are different. We see the issue only if the "Count" test is run after the custom extension test.
|
|
||
| // We create a new instance of ValidationRuleSet per call as a safeguard | ||
| // against unintentional modification of the private _defaultRuleSet. | ||
| return new ValidationRuleSet(_defaultRuleSet); |
There was a problem hiding this comment.
return new ValidationRuleSet(_defaultRuleSet); [](start = 4, length = 54)
We could alternatively prescribe that the caller never modifies the DefaultRuleSet and always uses it like this if they need to use default rules + more rules:
new ValidationRuleSet(ValidationRuleSet.DefaultRuleSet)
But then that's kinda risky and people will not always follow the best practice.
Creating the new instance here safeguards against that scenario as well.
There was a problem hiding this comment.
Creating the new collection on each call seems like the best approach. The rules don't need to get re-instantiated so the cost should be minimal. The other option would be to return a ReadOnlyCollection.
BTW, thanks for finding this. I was looking in the completely wrong direction.
There was a problem hiding this comment.
No problem.
A ReadOnlyCollection is something I wanted to do, but it will require more changes since we return the default rule set now as a class object, not as a list/dictionary of rules. I'll go with the current implementation for now, and we can revisit if this turns out not working well.
2 participants
Re-enable the default rule count test case (disabled in #176)
The problem was this:
ValidateCustomExtension test calls ValidationRuleSet.DefaultRuleSet and makes changes to it directly. Since the DefaultRuleSet is cached, the underlying private
_defaultRuleSetis actually modified.The reason we did not see the issue locally might be that the order of local test run and the order on AppVeyor are different. We see the issue only if the "Count" test is run after the custom extension test.