From f2bdba71379ef2d9e3f5c664bce8a5060ecaabc1 Mon Sep 17 00:00:00 2001 From: Gas <86567384+gas1cent@users.noreply.github.com> Date: Wed, 7 Feb 2024 14:16:24 +0400 Subject: [PATCH] test: improve test suite (#20) --- .github/workflows/test.yml | 2 +- .npmignore | 2 - src/processor.ts | 7 +- .../contracts}/BasicSample.sol | 0 .../contracts}/InterfacedSample.sol | 0 .../contracts}/LibrarySample.sol | 0 .../contracts}/ParserTest.sol | 0 .../ignored-data/IgnoredContract.sol | 0 .../contracts}/tests/BasicTest.t.sol | 0 test/mocks.ts | 6 +- test/parser.test.ts | 6 +- test/processor.test.ts | 114 ++++---- test/utils.ts | 4 + test/validator.test.ts | 257 ++++++------------ 14 files changed, 164 insertions(+), 234 deletions(-) rename {sample-data => test/contracts}/BasicSample.sol (100%) rename {sample-data => test/contracts}/InterfacedSample.sol (100%) rename {sample-data => test/contracts}/LibrarySample.sol (100%) rename {sample-data => test/contracts}/ParserTest.sol (100%) rename {sample-data => test/contracts}/ignored-data/IgnoredContract.sol (100%) rename {sample-data => test/contracts}/tests/BasicTest.t.sol (100%) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 06a3e61..f0dec0b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,7 +13,7 @@ jobs: strategy: matrix: - node-version: [16.x, 18.x] + node-version: [18.x, 20.x] steps: - uses: actions/checkout@v3 diff --git a/.npmignore b/.npmignore index 0dab6cb..1f3147a 100644 --- a/.npmignore +++ b/.npmignore @@ -1,5 +1,3 @@ .github .husky - test -sample-data diff --git a/src/processor.ts b/src/processor.ts index c022000..5d91a02 100644 --- a/src/processor.ts +++ b/src/processor.ts @@ -30,7 +30,7 @@ export class Processor { if (messages.length) { // Add the warning messages to the list together with the natspec location warnings.push({ - location: this.formatLocation(sourceUnit.absolutePath, fileContent, contract, node), + location: this.formatLocation(sourceUnit.absolutePath, fileContent, contract.name, node), messages, }); } @@ -54,15 +54,14 @@ export class Processor { } validateNatspec(node: NodeToProcess): string[] { - if (!node) return []; const nodeNatspec = parseNodeNatspec(node); return this.validator.validate(node, nodeNatspec); } - formatLocation(filePath: string, fileContent: string, contract: ContractDefinition, node: NodeToProcess): string { + formatLocation(filePath: string, fileContent: string, contractName: string, node: NodeToProcess): string { // the constructor function definition does not have a name, but it has kind: 'constructor' const nodeName = node instanceof FunctionDefinition ? node.name || node.kind : node.name; const line: number = getLineNumberFromSrc(fileContent, node.src); - return `${filePath}:${line}\n${contract.name}:${nodeName}`; + return `${filePath}:${line}\n${contractName}:${nodeName}`; } } diff --git a/sample-data/BasicSample.sol b/test/contracts/BasicSample.sol similarity index 100% rename from sample-data/BasicSample.sol rename to test/contracts/BasicSample.sol diff --git a/sample-data/InterfacedSample.sol b/test/contracts/InterfacedSample.sol similarity index 100% rename from sample-data/InterfacedSample.sol rename to test/contracts/InterfacedSample.sol diff --git a/sample-data/LibrarySample.sol b/test/contracts/LibrarySample.sol similarity index 100% rename from sample-data/LibrarySample.sol rename to test/contracts/LibrarySample.sol diff --git a/sample-data/ParserTest.sol b/test/contracts/ParserTest.sol similarity index 100% rename from sample-data/ParserTest.sol rename to test/contracts/ParserTest.sol diff --git a/sample-data/ignored-data/IgnoredContract.sol b/test/contracts/ignored-data/IgnoredContract.sol similarity index 100% rename from sample-data/ignored-data/IgnoredContract.sol rename to test/contracts/ignored-data/IgnoredContract.sol diff --git a/sample-data/tests/BasicTest.t.sol b/test/contracts/tests/BasicTest.t.sol similarity index 100% rename from sample-data/tests/BasicTest.t.sol rename to test/contracts/tests/BasicTest.t.sol diff --git a/test/mocks.ts b/test/mocks.ts index 0f914e8..e377f3d 100644 --- a/test/mocks.ts +++ b/test/mocks.ts @@ -1,5 +1,5 @@ import { SourceUnit, ContractDefinition, FunctionDefinition } from 'solc-typed-ast'; -import { Natspec, NodeToProcess } from '../src/types'; +import { Natspec, NodeToProcess, Config } from '../src/types'; export function mockNatspec(mockNatspec: Partial): Natspec { const natspec: Natspec = { @@ -31,3 +31,7 @@ export function mockSourceUnit(mockSourceUnit: Partial): SourceUnit export function mockContractDefinition(mockContractDefinition: Partial): ContractDefinition { return mockContractDefinition as ContractDefinition; } + +export function mockConfig(mockConfig: Partial): Config { + return mockConfig as Config; +} diff --git a/test/parser.test.ts b/test/parser.test.ts index ef3a2b0..86e68c5 100644 --- a/test/parser.test.ts +++ b/test/parser.test.ts @@ -8,7 +8,7 @@ describe('Parser', () => { describe('Contract', () => { beforeAll(async () => { - const compileResult = await getFileCompiledSource('sample-data/ParserTest.sol'); + const compileResult = await getFileCompiledSource('test/contracts/ParserTest.sol'); contract = compileResult.vContracts.find(({ name }) => name === 'ParserTest')!; }); @@ -167,7 +167,7 @@ describe('Parser', () => { describe('Interface', () => { beforeAll(async () => { - const compileResult = await getFileCompiledSource('sample-data/ParserTest.sol'); + const compileResult = await getFileCompiledSource('test/contracts/ParserTest.sol'); contract = compileResult.vContracts.find(({ name }) => name === 'IParserTest')!; }); @@ -302,7 +302,7 @@ describe('Parser', () => { describe('Contract with invalid natspec', () => { beforeAll(async () => { - const compileResult = await getFileCompiledSource('sample-data/ParserTest.sol'); + const compileResult = await getFileCompiledSource('test/contracts/ParserTest.sol'); contract = compileResult.vContracts.find(({ name }) => name === 'ParserTestFunny')!; }); diff --git a/test/processor.test.ts b/test/processor.test.ts index ee0148c..e0638d0 100644 --- a/test/processor.test.ts +++ b/test/processor.test.ts @@ -1,61 +1,20 @@ -import { ContractDefinition, FunctionDefinition, UserDefinedType, UsingForDirective } from 'solc-typed-ast'; -import { Config } from '../src/types'; -import { getFileCompiledSource } from './utils'; +import { faker } from '@faker-js/faker'; +import { ContractDefinition, FunctionDefinition, UserDefinedType, UsingForDirective, FunctionKind } from 'solc-typed-ast'; +import * as utils from '../src/utils'; import { Processor } from '../src/processor'; import { Validator } from '../src/validator'; +import { getFileCompiledSource } from './utils'; +import { mockFunctionDefinition, mockNodeToProcess, mockConfig, mockNatspec } from './mocks'; describe('Processor', () => { - let processor: Processor; - - beforeAll(() => { - const config: Config = { - root: '.', - include: './sample-data', - exclude: '', - enforceInheritdoc: false, - constructorNatspec: false, - }; - - const validator = new Validator(config); - processor = new Processor(validator); - }); - - // TODO: Fix these tests - // describe('formatLocation', () => { - // const absolutePath = faker.system.filePath(); - // const contractName = faker.lorem.word(); - // const nodeName = faker.lorem.word(); - - // const sourceUnit = mockSourceUnit({ - // absolutePath: absolutePath, - // }); - - // const contract = mockContractDefinition({ - // name: contractName, - // }); - - // it('should format the location of the node', () => { - // const node = mockNodeToProcess({ - // name: nodeName, - // }); - - // expect(processor.formatLocation(node, sourceUnit, contract)).toEqual(`${absolutePath}\n${contractName}:${nodeName}`); - // }); - - // it('should format the location of a constructor', () => { - // const node = mockFunctionDefinition({ - // kind: FunctionKind.Constructor, - // }); - - // expect(processor.formatLocation(node, sourceUnit, contract)).toEqual(`${absolutePath}\n${contractName}:constructor`); - // }); - // }); + const validator = new Validator(mockConfig({})); + const processor = new Processor(validator); describe('selectEligibleNodes', () => { let contract: ContractDefinition; beforeAll(async () => { - const compileResult = await getFileCompiledSource('sample-data/ParserTest.sol'); + const compileResult = await getFileCompiledSource('test/contracts/ParserTest.sol'); contract = compileResult.vContracts.find(({ name }) => name === 'ParserTest')!; }); @@ -130,4 +89,61 @@ describe('Processor', () => { expect(eligibleNodes.filter((node) => node instanceof FunctionDefinition && node.isConstructor).length).toEqual(1); }); }); + + describe('validateNatspec', () => { + const node = mockNodeToProcess({}); + const nodeNatspec = mockNatspec({}); + + it('should parse the natspec of the node', () => { + const parseNodeNatspecSpy = jest.spyOn(utils, 'parseNodeNatspec').mockImplementation((_) => nodeNatspec); + + processor.validateNatspec(node); + + expect(parseNodeNatspecSpy).toHaveBeenCalledWith(node); + }); + + it('should validate the natspec of the node', () => { + const messages = [faker.lorem.sentence(), faker.lorem.sentence()]; + const validateSpy = jest.spyOn(validator, 'validate').mockImplementation((_, __) => messages); + + const validatedNatspec = processor.validateNatspec(node); + + expect(validatedNatspec).toEqual(messages); + expect(validateSpy).toHaveBeenCalledWith(node, nodeNatspec); + }); + }); + + describe('formatLocation', () => { + const absolutePath = faker.system.filePath(); + const contractName = faker.lorem.word(); + const nodeName = faker.lorem.word(); + const fileContent = faker.lorem.sentence(); + const lineNumber = faker.number.int(100); + const src = '${lineNumber}:1:0'; + const getLineNumberFromSrcSpy = jest.spyOn(utils, 'getLineNumberFromSrc').mockImplementation(() => lineNumber); + + it('should format the location of a node', () => { + const node = mockNodeToProcess({ name: nodeName, src: src }); + const formattedLocation = processor.formatLocation(absolutePath, fileContent, contractName, node); + + expect(getLineNumberFromSrcSpy).toHaveBeenCalledWith(fileContent, src); + expect(formattedLocation).toEqual(`${absolutePath}:${lineNumber}\n${contractName}:${nodeName}`); + }); + + it('should format the location of a constructor', () => { + const node = mockFunctionDefinition({ kind: FunctionKind.Constructor, src: src }); + const formattedLocation = processor.formatLocation(absolutePath, fileContent, contractName, node); + + expect(getLineNumberFromSrcSpy).toHaveBeenCalledWith(fileContent, src); + expect(formattedLocation).toEqual(`${absolutePath}:${lineNumber}\n${contractName}:constructor`); + }); + + it('should format the location of a function', () => { + const node = mockFunctionDefinition({ name: nodeName, src: src }); + const formattedLocation = processor.formatLocation(absolutePath, fileContent, contractName, node); + + expect(getLineNumberFromSrcSpy).toHaveBeenCalledWith(fileContent, src); + expect(formattedLocation).toEqual(`${absolutePath}:${lineNumber}\n${contractName}:${nodeName}`); + }); + }); }); diff --git a/test/utils.ts b/test/utils.ts index 8bd6662..5778b87 100644 --- a/test/utils.ts +++ b/test/utils.ts @@ -4,3 +4,7 @@ export async function getFileCompiledSource(filePath: string): Promise warning.match(tag)).length).toBe(numberOfWarnings); +} diff --git a/test/validator.test.ts b/test/validator.test.ts index 2b1d048..f2bfa20 100644 --- a/test/validator.test.ts +++ b/test/validator.test.ts @@ -1,79 +1,37 @@ import { ContractDefinition } from 'solc-typed-ast'; import { Validator } from '../src/validator'; -import { getFileCompiledSource } from './utils'; -import { Config, NodeToProcess } from '../src/types'; -import { before } from 'node:test'; +import { NodeToProcess } from '../src/types'; +import { getFileCompiledSource, expectMissingTags } from './utils'; +import { mockConfig, mockNatspec } from './mocks'; describe('Validator', () => { let contract: ContractDefinition; let node: NodeToProcess; - - let config: Config = { - root: '.', - include: './sample-data', - exclude: '', - enforceInheritdoc: false, - constructorNatspec: false, - }; - - let validator: Validator = new Validator(config); + let validator: Validator = new Validator(mockConfig({})); beforeAll(async () => { - const compileResult = await getFileCompiledSource('sample-data/BasicSample.sol'); + const compileResult = await getFileCompiledSource('test/contracts/BasicSample.sol'); contract = compileResult.vContracts.find(({ name }) => name === 'BasicSample')!; }); - let natspec = { - tags: [ - { - name: 'notice', - content: 'External function that returns a bool', - }, - { - name: 'dev', - content: 'A dev comment', - }, - ], - params: [ - { - name: '_magicNumber', - content: 'A parameter description', - }, - { - name: '_name', - content: 'Another parameter description', - }, - ], - returns: [ - { - name: '_isMagic', - content: 'Some return data', - }, - { - name: undefined, - content: 'Test test', - }, - ], - }; - it('should validate proper natspec', () => { node = contract.vFunctions.find(({ name }) => name === 'externalSimple')!; - - const result = validator.validate(node, natspec); - expect(result).toEqual([]); - }); - - it('should reveal missing natspec for parameters', () => { - node = contract.vFunctions.find(({ name }) => name === 'externalSimple')!; - const paramName = '_magicNumber'; - let natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'External function that returns a bool', }, + { + name: 'dev', + content: 'A dev comment', + }, ], params: [ + { + name: '_magicNumber', + content: 'A parameter description', + }, { name: '_name', content: 'Another parameter description', @@ -84,25 +42,46 @@ describe('Validator', () => { name: '_isMagic', content: 'Some return data', }, + { + name: undefined, + content: 'Test test', + }, ], - }; + }); + const result = validator.validate(node, natspec); + expect(result).toEqual([]); + }); + it('should reveal missing natspec for parameters', () => { + node = contract.vFunctions.find(({ name }) => name === 'externalSimple')!; + const paramName = '_magicNumber'; + const natspec = mockNatspec({ + tags: [ + { + name: 'notice', + content: 'External function that returns a bool', + }, + ], + params: [ + { + name: '_name', + content: 'Another parameter description', + }, + ], + }); const result = validator.validate(node, natspec); + expectMissingTags(result, '@param', 1); expect(result).toContainEqual(`@param ${paramName} is missing`); }); it('should reveal missing natspec for returned values', () => { const paramName = '_isMagic'; - let natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'External function that returns a bool', }, - { - name: 'dev', - content: 'A dev comment', - }, ], params: [ { @@ -114,35 +93,20 @@ describe('Validator', () => { content: 'Another parameter description', }, ], - returns: [], - }; - + }); const result = validator.validate(node, natspec); + expectMissingTags(result, '@return', 1); expect(result).toContainEqual(`@return ${paramName} is missing`); }); it('should reveal missing natspec for unnamed returned values', () => { node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleReturn')!; - let natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'External function that returns a bool', }, - { - name: 'dev', - content: 'A dev comment', - }, - ], - params: [ - { - name: '_magicNumber', - content: 'A parameter description', - }, - { - name: '_name', - content: 'Another parameter description', - }, ], returns: [ { @@ -150,60 +114,43 @@ describe('Validator', () => { content: 'Some return data', }, ], - }; + }); const result = validator.validate(node, natspec); + expectMissingTags(result, '@return', 1); expect(result).toContainEqual(`@return missing for unnamed return №2`); }); it('should warn of a missing unnamed return', () => { node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleUnnamedReturn')!; - let natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'External function that returns a bool', }, - { - name: 'dev', - content: 'A dev comment', - }, ], - params: [], returns: [ { name: 'Some', content: 'return data', }, ], - }; + }); const result = validator.validate(node, natspec); - expect(result).toEqual([`@return missing for unnamed return №2`]); // only 1 warning + expectMissingTags(result, '@return', 1); + expect(result).toContainEqual(`@return missing for unnamed return №2`); }); it('should warn all returns if the first natspec tag is missing', () => { node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleReturn')!; - let natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'External function that returns a bool', }, - { - name: 'dev', - content: 'A dev comment', - }, - ], - params: [ - { - name: '_magicNumber', - content: 'A parameter description', - }, - { - name: '_name', - content: 'Another parameter description', - }, ], returns: [ { @@ -211,34 +158,22 @@ describe('Validator', () => { content: 'return data', }, ], - }; + }); const result = validator.validate(node, natspec); - expect(result).toEqual(['@return _isMagic is missing', '@return missing for unnamed return №2']); // 2 warnings + expectMissingTags(result, '@return', 2); + expect(result).toContainEqual(`@return _isMagic is missing`); + expect(result).toContainEqual(`@return missing for unnamed return №2`); }); it('should warn if the last natspec tag is missing', () => { node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleReturn')!; - let natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'External function that returns a bool', }, - { - name: 'dev', - content: 'A dev comment', - }, - ], - params: [ - { - name: '_magicNumber', - content: 'A parameter description', - }, - { - name: '_name', - content: 'Another parameter description', - }, ], returns: [ { @@ -246,10 +181,11 @@ describe('Validator', () => { content: 'Some return data', }, ], - }; + }); const result = validator.validate(node, natspec); - expect(result).toEqual(['@return missing for unnamed return №2']); // 1 warnings + expectMissingTags(result, '@return', 1); + expect(result).toContainEqual(`@return missing for unnamed return №2`); }); // TODO: Check overridden functions, virtual, etc? @@ -260,62 +196,53 @@ describe('Validator', () => { it('should reveal missing natspec for a variable', () => { node = contract.vStateVariables.find(({ name }) => name === '_EMPTY_STRING')!; - natspec = { - tags: [], - params: [], - returns: [], - }; - const result = validator.validate(node, natspec); + const result = validator.validate(node, mockNatspec({})); expect(result).toContainEqual(`Natspec is missing`); }); it('should reveal missing natspec for an error', () => { node = contract.vErrors.find(({ name }) => name === 'BasicSample_SomeError')!; const paramName = '_param1'; - natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'Some error missing parameter natspec', }, ], - params: [], - returns: [], - }; + }); const result = validator.validate(node, natspec); + expectMissingTags(result, '@param', 1); expect(result).toContainEqual(`@param ${paramName} is missing`); }); it('should reveal missing natspec for an event', () => { node = contract.vEvents.find(({ name }) => name === 'BasicSample_BasicEvent')!; const paramName = '_param1'; - natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'An event missing parameter natspec', }, ], - params: [], - returns: [], - }; + }); const result = validator.validate(node, natspec); + expectMissingTags(result, '@param', 1); expect(result).toContainEqual(`@param ${paramName} is missing`); }); it('should reveal missing natspec for an modifier', () => { node = contract.vModifiers.find(({ name }) => name === 'transferFee')!; const paramName = '_receiver'; - natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'Modifier notice', }, ], - params: [], - returns: [], - }; + }); const result = validator.validate(node, natspec); expect(result).toContainEqual(`@param ${paramName} is missing`); }); @@ -324,64 +251,46 @@ describe('Validator', () => { node = contract.vStructs.find(({ name }) => name === 'TestStruct')!; const paramName1 = 'someAddress'; const paramName2 = 'someNumber'; - natspec = { + const natspec = mockNatspec({ tags: [ { name: 'notice', content: 'Modifier notice', }, ], - params: [], - returns: [], - }; + }); const result = validator.validate(node, natspec); + expectMissingTags(result, '@param', 2); expect(result).toContainEqual(`@param ${paramName1} is missing`); expect(result).toContainEqual(`@param ${paramName2} is missing`); }); - it('should ignore receive and fallback', () => { - node = contract.vFunctions.find(({ kind }) => kind === 'receive' || kind === 'fallback')!; - natspec = { - tags: [], - params: [], - returns: [], - }; - const result = validator.validate(node, natspec); + it('should ignore the receive function', () => { + node = contract.vFunctions.find(({ kind }) => kind === 'receive')!; + const result = validator.validate(node, mockNatspec({})); + expect(result).toEqual([]); + }); + + it('should ignore the callback function', () => { + node = contract.vFunctions.find(({ kind }) => kind === 'fallback')!; + const result = validator.validate(node, mockNatspec({})); expect(result).toEqual([]); }); describe('with enforced inheritdoc', () => { beforeAll(async () => { - config = { - root: '.', - include: './sample-data', - exclude: '', - enforceInheritdoc: true, - constructorNatspec: false, - }; - - validator = new Validator(config); + validator = new Validator(mockConfig({ enforceInheritdoc: true })); }); it('should reveal missing inheritdoc for an overridden function', () => { node = contract.vFunctions.find(({ name }) => name === 'overriddenFunction')!; - natspec = { - tags: [], - params: [], - returns: [], - }; - const result = validator.validate(node, natspec); + const result = validator.validate(node, mockNatspec({})); expect(result).toContainEqual(`@inheritdoc is missing`); }); it('should reveal missing inheritdoc for a virtual function', () => { node = contract.vFunctions.find(({ name }) => name === 'virtualFunction')!; - natspec = { - tags: [], - params: [], - returns: [], - }; - const result = validator.validate(node, natspec); + const result = validator.validate(node, mockNatspec({})); expect(result).toContainEqual(`@inheritdoc is missing`); }); });