Repository: stoplightio/spectral
Pull request #723 (2019-10-31 12:11:38)

fix: inheritance of rule with implicit severity and a few extra tests covering inheritance
Related to https://github.com/stoplightio/spectral/issues/447

**Checklist**

- [x] Tests added / updated
- [x] Docs added / updated

**Does this PR introduce a breaking change?**

- [ ] Yes
- [x] No

**Additional context**
- [ ] help me with naming :sweat_smile: 


philsturgeon: Is there any way we can test a bit more directly? Instead of testing the results are trimmed in a likely way, can we test to see if it is passing a "off" or a "recommended" to some function? Basically make this a bit more of a unit test than an integration test? It's tough to look at this and see exactly what is going on. 
P0lip: > Is there any way we can test a bit more directly? Instead of testing the results are trimmed in a likely way, can we test to see if it is passing a "off" or a "recommended" to some function?

Yeah, you are right. I find them quite difficult to follow as well when I update them in future, so I'm eager to add a few unit tests.
That said, I'd still like to keep these integration tests, as they give me more confidence.
Are you okay with that?
philsturgeon: Sure, both is good.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, November 14, 2019 4:41 PM, Jakub Rożek <notifications@github.com> wrote:

>> Is there any way we can test a bit more directly? Instead of testing the results are trimmed in a likely way, can we test to see if it is passing a "off" or a "recommended" to some function?
>
> Yeah, you are right. I find them quite difficult to follow as well when I update them in future, so I'm eager to add a few unit tests.
> That said, I'd still like to keep these integration tests, as they give me more confidence.
> Are you okay with that?
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, [view it on GitHub](https://github.com/stoplightio/spectral/pull/723?email_source=notifications&email_token=AAAQONOH6LBJXYGSVE2ZCEDQTV5SLA5CNFSM4JHJANA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEECPEYQ#issuecomment-553972322), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAAQONOELRVCQP747DJTY5TQTV5SLANCNFSM4JHJANAQ).
P0lip: @philsturgeon was writing a few unit tests, and spotted a case we don't support well
- contents of `foo.json` (rule without any explicit severity)
```json
{
  "rules": {
    "optional-rule": {
      "message": "foo",
      "given": "",
      "then": {
        "function": ""
      }
    }
}
```
- ruleset extending foo.json
```json
{
  "extends": [["./foo.json", "off"]],
  "rules": {
    "optional-rule": true
  }
}
```
Gonna fix this minor issue in this PR.
Initially I thought my unit test is just broken, but turned out I missed this one.
P0lip: @marbemac @philsturgeon alright fellas. I won't come with any more tests, I believe.
I added some unit tests for rules mergers we didn't have, but besides these, we already have quite a few tests, therefore I cannot really think of anything else I'd like to cover.
If you have any ideas, I'm all ears.

Patch:
diff --git a/src/rulesets/mergers/rules.ts b/src/rulesets/mergers/rules.ts
index 0626f621..3e4f152c 100644
--- a/src/rulesets/mergers/rules.ts
+++ b/src/rulesets/mergers/rules.ts
@@ -53,7 +53,7 @@ function updateRootRule(root: Rule, newRule: Rule | null) {
   Object.assign(root[ROOT_DESCRIPTOR], copyRule(newRule === null ? root : Object.assign(root, newRule)));
 }
 
-function getRootRule(rule: Rule): Rule {
+function getRootRule(rule: Rule): Rule | null {
   return rule[ROOT_DESCRIPTOR] !== undefined ? rule[ROOT_DESCRIPTOR] : null;
 }
 
@@ -68,11 +68,16 @@ function processRule(rules: FileRuleCollection, name: string, rule: FileRule | F
     case 'boolean':
       if (isValidRule(existingRule)) {
         const rootRule = getRootRule(existingRule);
-        if (rule) {
-          existingRule.severity = rootRule ? rootRule.severity : getSeverityLevel(rules, name, rule);
+        if (!rule) {
+          existingRule.severity = -1;
+        } else if (rootRule === null) {
+          existingRule.severity = getSeverityLevel(rules, name, rule);
+          updateRootRule(existingRule, existingRule);
+        } else if ('severity' in rootRule) {
+          existingRule.severity = rootRule.severity;
           updateRootRule(existingRule, existingRule);
         } else {
-          existingRule.severity = -1;
+          existingRule.severity = DiagnosticSeverity.Warning;
         }
       }
       break;


Test patch:
diff --git a/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/my-ruleset-recommended.json b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/my-ruleset-recommended.json
new file mode 100644
index 00000000..9c704522
--- /dev/null
+++ b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/my-ruleset-recommended.json
@@ -0,0 +1,3 @@
+{
+  "extends": [["./my-ruleset.json", "recommended"]]
+}
diff --git a/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/my-ruleset.json b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/my-ruleset.json
new file mode 100644
index 00000000..36a45660
--- /dev/null
+++ b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/my-ruleset.json
@@ -0,0 +1,3 @@
+{
+  "extends": [["./ruleset-c.json", "all"]]
+}
diff --git a/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-a.json b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-a.json
new file mode 100644
index 00000000..1f5deb95
--- /dev/null
+++ b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-a.json
@@ -0,0 +1,43 @@
+{
+  "rules": {
+    "description-matches-stoplight": {
+      "message": "Description must contain Stoplight",
+      "given": "$.info",
+      "type": "style",
+      "recommended": true,
+      "severity": "error",
+      "then": {
+        "field": "description",
+        "function": "pattern",
+        "functionOptions": {
+          "match": "Stoplight"
+        }
+      }
+    },
+    "title-matches-stoplight": {
+      "message": "Title must contain Stoplight",
+      "given": "$.info",
+      "type": "style",
+      "then": {
+        "field": "title",
+        "function": "pattern",
+        "functionOptions": {
+          "match": "Stoplight"
+        }
+      }
+    },
+    "contact-name-matches-stoplight": {
+      "message": "Contact name must contain Stoplight",
+      "given": "$.info.contact",
+      "type": "style",
+      "recommended": false,
+      "then": {
+        "field": "name",
+        "function": "pattern",
+        "functionOptions": {
+          "match": "Stoplight"
+        }
+      }
+    }
+  }
+}
diff --git a/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-b.json b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-b.json
new file mode 100644
index 00000000..e577d214
--- /dev/null
+++ b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-b.json
@@ -0,0 +1,3 @@
+{
+  "extends": "./ruleset-a.json"
+}
diff --git a/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-c.json b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-c.json
new file mode 100644
index 00000000..6ec51ead
--- /dev/null
+++ b/src/rulesets/__tests__/__fixtures__/inheritanceRulesets/ruleset-c.json
@@ -0,0 +1,3 @@
+{
+  "extends": [["./ruleset-b.json", "off"]]
+}
diff --git a/src/rulesets/__tests__/reader.jest.test.ts b/src/rulesets/__tests__/reader.jest.test.ts
index 7fe4c948..d69c24ac 100644
--- a/src/rulesets/__tests__/reader.jest.test.ts
+++ b/src/rulesets/__tests__/reader.jest.test.ts
@@ -402,6 +402,159 @@ describe('Rulesets reader', () => {
     );
   });
 
+  it('given ruleset with extends set to all, should enable all rules', () => {
+    return expect(
+      readRuleset(path.join(__dirname, './__fixtures__/inheritanceRulesets/my-ruleset.json')),
+    ).resolves.toStrictEqual({
+      functions: {},
+      rules: {
+        'contact-name-matches-stoplight': {
+          given: '$.info.contact',
+          message: 'Contact name must contain Stoplight',
+          recommended: false,
+          severity: DiagnosticSeverity.Warning,
+          then: {
+            field: 'name',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+        'description-matches-stoplight': {
+          given: '$.info',
+          message: 'Description must contain Stoplight',
+          severity: DiagnosticSeverity.Error,
+          recommended: true,
+          then: {
+            field: 'description',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+        'title-matches-stoplight': {
+          given: '$.info',
+          message: 'Title must contain Stoplight',
+          severity: DiagnosticSeverity.Warning,
+          then: {
+            field: 'title',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+      },
+    });
+  });
+
+  it('given ruleset with extends set to recommended, should enable recommended rules', () => {
+    return expect(
+      readRuleset(path.join(__dirname, './__fixtures__/inheritanceRulesets/my-ruleset-recommended.json')),
+    ).resolves.toStrictEqual({
+      functions: {},
+      rules: {
+        'contact-name-matches-stoplight': {
+          given: '$.info.contact',
+          message: 'Contact name must contain Stoplight',
+          recommended: false,
+          severity: -1,
+          then: {
+            field: 'name',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+        'description-matches-stoplight': {
+          given: '$.info',
+          message: 'Description must contain Stoplight',
+          severity: DiagnosticSeverity.Error,
+          recommended: true,
+          then: {
+            field: 'description',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+        'title-matches-stoplight': {
+          given: '$.info',
+          message: 'Title must contain Stoplight',
+          severity: DiagnosticSeverity.Warning,
+          then: {
+            field: 'title',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+      },
+    });
+  });
+
+  it('given ruleset with extends set to off, should disable all rules', () => {
+    return expect(
+      readRuleset(path.join(__dirname, './__fixtures__/inheritanceRulesets/ruleset-c.json')),
+    ).resolves.toStrictEqual({
+      functions: {},
+      rules: {
+        'contact-name-matches-stoplight': {
+          given: '$.info.contact',
+          message: 'Contact name must contain Stoplight',
+          recommended: false,
+          severity: -1,
+          then: {
+            field: 'name',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+        'description-matches-stoplight': {
+          given: '$.info',
+          message: 'Description must contain Stoplight',
+          severity: -1,
+          recommended: true,
+          then: {
+            field: 'description',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+        'title-matches-stoplight': {
+          given: '$.info',
+          message: 'Title must contain Stoplight',
+          severity: -1,
+          then: {
+            field: 'title',
+            function: 'pattern',
+            functionOptions: {
+              match: 'Stoplight',
+            },
+          },
+          type: 'style',
+        },
+      },
+    });
+  });
+
   it('should support local rulesets', () => {
     return expect(readRuleset(extendsRelativeRuleset)).resolves.toEqual(
       expect.objectContaining({
diff --git a/src/rulesets/__tests__/rule_name_conflicts.jest.test.ts b/src/rulesets/__tests__/rule-name-conflicts.jest.test.ts
similarity index 100%
rename from src/rulesets/__tests__/rule_name_conflicts.jest.test.ts
rename to src/rulesets/__tests__/rule-name-conflicts.jest.test.ts
diff --git a/src/rulesets/mergers/__tests__/rules.jest.test.ts b/src/rulesets/mergers/__tests__/rules.jest.test.ts
index 367c8ba8..f3009809 100644
--- a/src/rulesets/mergers/__tests__/rules.jest.test.ts
+++ b/src/rulesets/mergers/__tests__/rules.jest.test.ts
@@ -1,5 +1,6 @@
 import { DiagnosticSeverity } from '@stoplight/types';
 import { IRule } from '../../../types';
+import { FileRuleCollection } from '../../../types/ruleset';
 import { mergeRules } from '../rules';
 
 describe('Ruleset rules merging', () => {
@@ -146,18 +147,6 @@ describe('Ruleset rules merging', () => {
     expect(rules).toHaveProperty('test.severity', -1);
   });
 
-  it('supports array-ish syntax', () => {
-    const rules = {
-      test: JSON.parse(JSON.stringify(baseRule)),
-    };
-
-    mergeRules(rules, {
-      test: ['off'],
-    });
-
-    expect(rules).toHaveProperty('test.severity', -1);
-  });
-
   it('does not set functionOptions if rule does not implement it', () => {
     const rules = {
       test: JSON.parse(JSON.stringify(baseRule)),
@@ -324,6 +313,121 @@ describe('Ruleset rules merging', () => {
     expect(rules).toHaveProperty('test.severity', DiagnosticSeverity.Hint);
   });
 
+  describe('inheriting rules with no explicit severity levels', () => {
+    let rules: FileRuleCollection;
+
+    beforeEach(() => {
+      rules = {
+        rule: {
+          given: '',
+          then: { function: '' },
+          recommended: true,
+        },
+        'rule-with-no-recommended': {
+          given: '',
+          then: { function: '' },
+        },
+        'optional-rule': {
+          given: '',
+          then: { function: '' },
+          recommended: false,
+        },
+      };
+    });
+
+    it('sets warning as a default', () => {
+      const newRules = mergeRules({}, rules);
+
+      const custom = {
+        rule: true,
+        'rule-with-no-recommended': true,
+        'optional-rule': true,
+      };
+
+      expect(mergeRules(newRules, custom)).toEqual({
+        rule: expect.objectContaining({
+          recommended: true,
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'rule-with-no-recommended': expect.objectContaining({
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'optional-rule': expect.objectContaining({
+          recommended: false,
+          severity: DiagnosticSeverity.Warning,
+        }),
+      });
+    });
+
+    it('sets warning as a default even when all rules were disabled', () => {
+      const newRules = mergeRules({}, rules, 'off');
+
+      const custom = {
+        rule: true,
+        'rule-with-no-recommended': true,
+        'optional-rule': true,
+      };
+
+      expect(mergeRules(newRules, custom)).toEqual({
+        rule: expect.objectContaining({
+          recommended: true,
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'rule-with-no-recommended': expect.objectContaining({
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'optional-rule': expect.objectContaining({
+          recommended: false,
+          severity: DiagnosticSeverity.Warning,
+        }),
+      });
+    });
+
+    it('respects ruleset severity', () => {
+      expect(mergeRules({}, rules, 'all')).toEqual({
+        rule: expect.objectContaining({
+          recommended: true,
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'rule-with-no-recommended': expect.objectContaining({
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'optional-rule': expect.objectContaining({
+          recommended: false,
+          severity: DiagnosticSeverity.Warning,
+        }),
+      });
+
+      expect(mergeRules({}, rules, 'recommended')).toEqual({
+        rule: expect.objectContaining({
+          recommended: true,
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'rule-with-no-recommended': expect.objectContaining({
+          severity: DiagnosticSeverity.Warning,
+        }),
+        'optional-rule': expect.objectContaining({
+          recommended: false,
+          severity: -1,
+        }),
+      });
+
+      expect(mergeRules({}, rules, 'off')).toEqual({
+        rule: expect.objectContaining({
+          recommended: true,
+          severity: -1,
+        }),
+        'rule-with-no-recommended': expect.objectContaining({
+          severity: -1,
+        }),
+        'optional-rule': expect.objectContaining({
+          recommended: false,
+          severity: -1,
+        }),
+      });
+    });
+  });
+
   it('given invalid rule value should throw', () => {
     const rules = {
       test: JSON.parse(JSON.stringify(baseRule)),