Implement or function - #965
TimvdLippe wants to merge 1 commit into
Conversation
To mirror the implementation of the `or` function in Spectral. It succeeds if one of the properties is present. It requires at least two values.
| "then": { | ||
| "function": "xor", | ||
| "functionOptions" : { | ||
| "properties" : ["externalValue", "value"] |
There was a problem hiding this comment.
These were the exact same values as TestApplyRules_Xor_Success, so that was not intended.
| "functionOptions" : { | ||
| "properties" : ["externalValue", "value"] | ||
| "functionOptions" : { | ||
| "properties" : ["summary", "value"] |
There was a problem hiding this comment.
These are always both present, thus they should NOT pass the xor
|
|
||
| assert.Len(t, results.Results, 0) | ||
| assert.Len(t, results.Errors, 0) | ||
| assert.Len(t, results.Results, 0) // This is wrong, as the don't throw any error even if xor always reports |
There was a problem hiding this comment.
Despite the changes above to actually start failing the test, there are no results and Vacuum is happy. Even if I change the implementation of Xor to always add a result, this test still reports Results as []. Not sure what's going on and why none of the tests execute here
| results := ApplyRulesToRuleSet(rse) | ||
|
|
||
| assert.Len(t, results.Errors, 0) | ||
| assert.Len(t, results.Results, 0) // This is wrong, but the xor tests also don't throw any error |
There was a problem hiding this comment.
See comments below why this isn't working, but was already not correctly testing the xor function
There was a problem hiding this comment.
asserts zero violations while acknowledging that result is wrong. It should use a tiny deterministic fixture and require the expected violation count and path. The modified xor test has the same problem.
There was a problem hiding this comment.
Yes I know, but that's my question. Do you know why the pre-existing xor test is wrong and how to fix it? Then I can also fix this test. My debugging unfortunately didn't get me anywhere
daveshanley
left a comment
daveshanley
left a comment
There was a problem hiding this comment.
Nice, I was waiting for this to pop up.
| if len(props) <= 0 { | ||
| properties = utils.ConvertInterfaceToStringArray(context.Options) | ||
| } else { | ||
| properties = strings.Split(props["properties"], ",") |
There was a problem hiding this comment.
parsed YAML/JSON arrays are flattened to [title description], then split only on commas. Consequently, the documented properties: [title, description] form produces zero results. Spectral explicitly requires properties to be a string array
| results := ApplyRulesToRuleSet(rse) | ||
|
|
||
| assert.Len(t, results.Errors, 0) | ||
| assert.Len(t, results.Results, 0) // This is wrong, but the xor tests also don't throw any error |
There was a problem hiding this comment.
asserts zero violations while acknowledging that result is wrong. It should use a tiny deterministic fixture and require the expected violation count and path. The modified xor test has the same problem.
| builder.WriteString(field) | ||
| builder.WriteString("`") | ||
| } | ||
| builder.WriteString("` must be defined") |
There was a problem hiding this comment.
adds an extra backtick. The actual output is:
at least one of title description`` must be defined`
Properties also retain leading whitespace and lack readable separators.
| if len(props) <= 0 { | ||
| properties = utils.ConvertInterfaceToStringArray(context.Options) | ||
| } else { | ||
| properties = strings.Split(props["properties"], ",") |
There was a problem hiding this comment.
Please handle properties as an array so valid rulesets produce violations instead of silently returning.
| results := ApplyRulesToRuleSet(rse) | ||
|
|
||
| assert.Len(t, results.Errors, 0) | ||
| assert.Len(t, results.Results, 0) // This is wrong, but the xor tests also don't throw any error |
There was a problem hiding this comment.
Please use a tiny failing fixture here and assert the violation count and path.
| builder.WriteString(field) | ||
| builder.WriteString("`") | ||
| } | ||
| builder.WriteString("` must be defined") |
There was a problem hiding this comment.
Please trim and separate the property names, and remove the extra backtick from this message.
|
@daveshanley I am not sure if my comment was clear enough, but it seems like we are talking past each other. The test that I added is not asserting what it is supposed to assert. The reason is that I took the existing test that is there and discovered that test isn't working either. This means that the pre-existing tests don't assert properly. I have done my best to debug the tests and corresponding code, but had to give up after a while. Therefore, I am asking you to figure out why the pre-existing Unfortunately at this point I am unable to proceed with this PR and address your comments, as I lack the knowledge of the inner details of Vacuum to understand why those tests behave like they are. |
2 participants
To mirror the implementation of the
orfunction in Spectral. It succeeds if one of the properties is present. It requires at least two values.Fixes #964