Merge pull request #455 from protofire/i161-add-tests-max-warnings

I161 add e2e to test max warnings

.github/workflows/E2E.yml

@@ -14,7 +14,7 @@ jobs:
     name: Test on Linux with Node ${{ matrix.node }}
     strategy:
       matrix:
-        node: [14, 16, 18]
+        node: [16, 18, 20]
 
     steps:
       - uses: actions/checkout@v3
@@ -36,15 +36,21 @@ jobs:
 
       - name: Global Installation
         run: npm i -g solhint*tgz
+
+      - name: Check solhint version
+        run: solhint --version
 
       - name: Run E2E Tests
         run: cd e2e && npm install && npm test
 
   e2e_windows:
     runs-on: windows-latest
-    name: Run linter and E2E Tests on Windows
+    name: Test on Windows    
 
     steps:
+      - name: Enable Debugging
+        run: |
+          echo "::debug::Debugging enabled"
       - uses: actions/checkout@v3
       - uses: actions/setup-node@v3
         with:
@@ -53,8 +59,23 @@ jobs:
       - name: Install dependencies
         run: npm install --include=dev
 
-      - name: Install solhint
-        run: npm i -g solhint
+      - name: Run linter
+        run: npm run lint
+
+      - name: Generate Docs
+        run: npm run docs 
+
+      - name: Pack
+        run: npm pack
+
+      - name: Global Installation
+        run: npm i -g @(Get-ChildItem -Filter *.tgz)
+
+      - name: Check solhint version
+        run: solhint --version
+
+      - name: List directory contents
+        run: dir
 
       - name: Run linter
         run: npm run lint
@@ -64,7 +85,7 @@ jobs:
 
   e2e_macos:
     runs-on: macos-latest
-    name: Run linter and E2E Tests on MacOS
+    name: Test on MacOS
 
     steps:
     - uses: actions/checkout@v3

.github/workflows/TESTS.yml

@@ -16,7 +16,7 @@ jobs:
     name: Run linter and tests on Linux
     strategy:
       matrix:
-        node-version: [14, 16, 18]
+        node-version: [16, 18, 20]
 
     steps:
     - uses: actions/checkout@v3

README.md

@@ -67,6 +67,7 @@ Options:
 Commands:
 
   stdin [options]                         linting of source code data provided to STDIN
+  list-rules                              display covered rules of current .solhint.json
 ```
 ### Note
 The `--fix`  option currently works only on "avoid-throw" and "avoid-sha3" rules

e2e/05-max-warnings/.solhint.json

@@ -0,0 +1,3 @@
+{
+  "extends": "solhint:all"
+}

e2e/05-max-warnings/contracts/Foo.sol

@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.5.8;
+
+contract Foo {
+    uint256 public constant test1 = 1;
+    uint256 TEST2;
+
+    constructor() {
+
+    }
+}

e2e/05-max-warnings/contracts/Foo2.sol

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.6.0;
+
+contract Foo {
+    uint256 public constant test1 = 1;
+    uint256 TEST2;
+
+    constructor() {}
+
+    function TEST() {}
+
+    modifier ZAR_tok() {}
+
+    uint256 SOSO;
+}

e2e/test.js

@@ -1,7 +1,3 @@
-/* eslint-disable func-names */
-/* eslint-disable prefer-arrow-callback */
-/* eslint-disable no-unused-expressions */
-
 const { expect } = require('chai')
 const cp = require('child_process')
 const fs = require('fs-extra')
@@ -11,7 +7,7 @@ const path = require('path')
 const shell = require('shelljs')
 
 function useFixture(dir) {
-  beforeEach(`switch to ${dir}`, function() {
+  beforeEach(`switch to ${dir}`, function () {
     const fixturePath = path.join(__dirname, dir)
 
     const tmpDirContainer = os.tmpdir()
@@ -26,17 +22,17 @@ function useFixture(dir) {
   })
 }
 
-describe('e2e', function() {
-  describe('no config', function() {
+describe('e2e', function () {
+  describe('no config', function () {
     useFixture('01-no-config')
 
-    it('should fail', function() {
+    it('should fail', function () {
       const { code } = shell.exec('solhint Foo.sol')
 
       expect(code).to.equal(1)
     })
 
-    it('should create an initial config with --init', function() {
+    it('should create an initial config with --init', function () {
       const { code } = shell.exec('solhint --init')
 
       expect(code).to.equal(0)
@@ -46,55 +42,51 @@ describe('e2e', function() {
       expect(fs.existsSync(solhintConfigPath)).to.be.true
     })
 
-    it('should print usage if called without arguments', function() {
+    it('should print usage if called without arguments', function () {
       const { code, stdout } = shell.exec('solhint')
 
       expect(code).to.equal(0)
-
       expect(stdout).to.include('Usage: solhint [options]')
       expect(stdout).to.include('Linter for Solidity programming language')
       expect(stdout).to.include('linting of source code data provided to STDIN')
     })
   })
 
-  describe('empty-config', function() {
+  describe('empty-config', function () {
     useFixture('02-empty-solhint-json')
 
-    it('should print nothing', function() {
+    it('should print nothing', function () {
       const { code, stdout } = shell.exec('solhint Foo.sol')
 
       expect(code).to.equal(0)
-
       expect(stdout.trim()).to.equal('')
     })
 
-    it('should show warning when using --init', function() {
+    it('should show warning when using --init', function () {
       const { code, stdout } = shell.exec('solhint --init')
 
       expect(code).to.equal(0)
-
       expect(stdout.trim()).to.equal('Configuration file already exists')
     })
   })
 
-  describe('no-empty-blocks', function() {
+  describe('no-empty-blocks', function () {
     useFixture('03-no-empty-blocks')
 
-    it('should exit with 1', function() {
+    it('should exit with 1', function () {
       const { code, stdout } = shell.exec('solhint Foo.sol')
 
       expect(code).to.equal(1)
-
       expect(stdout.trim()).to.contain('Code contains empty blocks')
     })
 
-    it('should work with stdin', async function() {
+    it('should work with stdin', async function () {
       const child = cp.exec('solhint stdin')
 
       const stdoutPromise = getStream(child.stdout)
 
-      const codePromise = new Promise(resolve => {
-        child.on('close', code => {
+      const codePromise = new Promise((resolve) => {
+        child.on('close', (code) => {
           resolve(code)
         })
       })
@@ -112,13 +104,45 @@ describe('e2e', function() {
     })
   })
 
-  describe('.sol on path', function() {
+  describe('.sol on path', function () {
     useFixture('04-dotSol-on-path')
 
-    it('should handle directory names that end with .sol', function() {
+    it('should handle directory names that end with .sol', function () {
       const { code } = shell.exec('solhint contracts/**/*.sol')
+      expect(code).to.equal(0)
+    })
+  })
+
+  describe('--max-warnings parameter tests', function () {
+    // Foo contract has 6 warnings
+    // Foo2 contract has 1 error and 14 warnings
+    useFixture('05-max-warnings')
+    const warningExceededMsg = 'Solhint found more warnings than the maximum specified'
+    const errorFound =
+      'Error/s found on rules! [max-warnings] param is ignored. Fixing errors enables max-warnings'
 
+    it('should not display [warnings exceeded] for max 7 warnings', function () {
+      const { code, stdout } = shell.exec('solhint contracts/Foo.sol --max-warnings 7')
       expect(code).to.equal(0)
+      expect(stdout.trim()).to.not.contain(warningExceededMsg)
+    })
+
+    it('should display [warnings exceeded] for max 3 warnings and exit error 1', function () {
+      const { code, stdout,  } = shell.exec('solhint contracts/Foo.sol --max-warnings 3')
+      expect(code).to.equal(1)
+      expect(stdout.trim()).to.contain(warningExceededMsg)
+    })
+
+    it('should return error for Compiler version rule, ignoring 3 --max-warnings', function () {
+      const { code, stdout } = shell.exec('solhint contracts/Foo2.sol --max-warnings 3')      
+      expect(code).to.equal(1)
+      expect(stdout.trim()).to.contain(errorFound)
+    })
+
+    it('should return error for Compiler version rule. No message for max-warnings', function () {
+      const { code, stdout } = shell.exec('solhint contracts/Foo2.sol --max-warnings 27')
+      expect(code).to.equal(1)
+      expect(stdout.trim()).to.not.contain(errorFound)
     })
   })
 })

lib/rules/best-practises/reason-string.js

@@ -82,7 +82,12 @@ class ReasonStringChecker extends BaseChecker {
       // If has reason message and is too long, throw an error
       const reason = _.last(functionParameters).value
       if (reason.length > this.maxCharactersLong) {
-        this._errorMessageIsTooLong(node, functionName)
+        const infoMessage = reason.length
+          .toString()
+          .concat(' counted / ')
+          .concat(this.maxCharactersLong.toString())
+          .concat(' allowed')
+        this._errorMessageIsTooLong(node, functionName, infoMessage)
       }
     }
   }
@@ -103,8 +108,8 @@ class ReasonStringChecker extends BaseChecker {
     this.error(node, `Provide an error message for ${key}`)
   }
 
-  _errorMessageIsTooLong(node, key) {
-    this.error(node, `Error message for ${key} is too long`)
+  _errorMessageIsTooLong(node, key, infoMessage) {
+    this.error(node, `Error message for ${key} is too long: ${infoMessage}`)
   }
 }