From 47526daa1a786246f95c4a626fa7c1555a20c481 Mon Sep 17 00:00:00 2001 From: ConnectDotz Date: Sun, 1 Aug 2021 17:09:57 -0400 Subject: [PATCH] fix a few match issues and prepare for test provider support (#738) * fix a few match issues and prepare for test provider support --- CHANGELOG.md | 2 + src/TestResults/match-by-context.ts | 91 ++++++++++++++++------ src/TestResults/match-node.ts | 71 ++++++++++------- tests/TestResults/match-by-context.test.ts | 65 ++++++++++++++++ tests/TestResults/match-node.test.ts | 10 ++- 5 files changed, 182 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27674709e..3e6cfff70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ Bug-fixes within the same version aren't needed ## Master * fix type warning in settings.json when using the `{"autoRun": "off"}` option - @tommy +* fix couple of test result matching issues - @connectdotz (#737) +* update match API/attributes to support up-coming vscode test provider API. - @connectdotz (#737) --> diff --git a/src/TestResults/match-by-context.ts b/src/TestResults/match-by-context.ts index 068daf4d0..b94ced880 100644 --- a/src/TestResults/match-by-context.ts +++ b/src/TestResults/match-by-context.ts @@ -27,6 +27,8 @@ import { ROOT_NODE_NAME, OptionalAttributes, MatchEvent, + NodeType, + MatchOptions, } from './match-node'; export const buildAssertionContainer = ( @@ -46,7 +48,10 @@ export const buildAssertionContainer = ( new DataNode(a.title, zeroBasedLine, a, { fullName: a.fullName, isGroup: 'maybe', - range: { start: zeroBasedLine, end: zeroBasedLine }, + range: { + start: { line: zeroBasedLine, column: 0 }, + end: { line: zeroBasedLine, column: 0 }, + }, }) ); }); @@ -57,6 +62,7 @@ export const buildAssertionContainer = ( return root; }; +const UnknownRange = { start: { line: -1, column: -1 }, end: { line: -1, column: -1 } }; export const buildSourceContainer = (sourceRoot: ParsedNode): ContainerNode => { const isDescribeBlock = (node: ParsedNode): node is DescribeBlock => node.type === 'describe'; const isItBlock = (node: ParsedNode): node is ItBlock => node.type === 'it'; @@ -65,7 +71,10 @@ export const buildSourceContainer = (sourceRoot: ParsedNode): ContainerNode ({ isGroup: namedNode.lastProperty === 'each' ? 'yes' : 'no', nonLiteralName: namedNode.nameType !== 'Literal', - range: { start: namedNode.start?.line - 1 ?? -1, end: namedNode.end?.line - 1 ?? -1 }, + range: { + start: namedNode.start ? adjustLocation(namedNode.start) : UnknownRange.start, + end: namedNode.end ? adjustLocation(namedNode.end) : UnknownRange.end, + }, }); if (isDescribeBlock(node)) { container = new ContainerNode(node.name, attrs(node)); @@ -95,7 +104,7 @@ const matchPos = (t: ItBlock, a: TestAssertionStatus, forError = false): boolean }; // could not use "instanceof" check as it could fail tests that mocked jest-editor-support like in TestResultProvider.test.ts -const isDataNode = (arg: DataNode | ItBlock): arg is DataNode => +const isSourceDataNode = (arg: DataNode | ItBlock): arg is DataNode => // eslint-disable-next-line @typescript-eslint/no-explicit-any (arg as any).data; @@ -104,7 +113,7 @@ export const toMatchResult = ( assertionOrErr: DataNode | string, reason: MatchEvent ): TestResult => { - const [test, sourceHistory, sourceName] = isDataNode(source) + const [test, sourceHistory, sourceName] = isSourceDataNode(source) ? [source.data, source.history(reason), source.fullName] : [source, [reason], source.name]; const [assertion, assertionHistory, err] = @@ -178,9 +187,27 @@ interface FallbackMatchResult { type ClassicMatchType = 'by-name' | 'by-location'; const ContextMatch = (): ContextMatchAlgorithm => { + const onMatch = ( + tNode: NodeType, + aNode: NodeType, + event: MatchEvent + ): void => { + tNode.addEvent(event); + aNode.addEvent(event); + aNode.attrs.range = tNode.attrs.range; + }; + const onMatchResult = ( + tNode: DataNode, + aNode: DataNode, + event: MatchEvent + ): TestResult => { + onMatch(tNode, aNode, event); + return toMatchResult(tNode, aNode, event); + }; const handleTestBlockMatch = ( result: MatchResultType<'data'>, - reportUnmatch: boolean + reportUnmatch = false, + matchGroup = true ): TestResult[] => { const [t, matched, reason] = result; if (matched.length === 0) { @@ -190,31 +217,45 @@ const ContextMatch = (): ContextMatchAlgorithm => { return []; } - return matched.flatMap((a) => a.getAll().map((aa) => toMatchResult(t, aa, reason))); + return matched.flatMap((a) => + matchGroup + ? a.getAll().map((aa) => onMatchResult(t, aa, reason)) + : onMatchResult(t, a, reason) + ); }; - const handleDescribeBlockMatch = (result: MatchResultType<'container'>): TestResult[] => { + const handleDescribeBlockMatch = ( + result: MatchResultType<'container'>, + matchGroup = true + ): TestResult[] => { const [t, matched, reason] = result; if (matched.length === 0) { return []; } - t.addEvent(reason); - return matched.flatMap((a) => { - a.addEvent(reason); + const _matchContainers = (a: ContainerNode) => { + onMatch(t, a, reason); return matchContainers(t, a); - }); + }; + return matched.flatMap((a) => + matchGroup ? a.getAll().flatMap(_matchContainers) : _matchContainers(a) + ); }; // match methods + const matchByNameOptions: MatchOptions = { ignoreGroupDiff: true }; + const matchByLocationOptions: MatchOptions = { + checkIsWithin: true, + ignoreNonLiteralNameDiff: true, + ignoreGroupDiff: true, + }; const classicMatch = ( type: ClassicMatchType, tList: ChildNodeType[], aList: ChildNodeType[] ): MatchMethodResult => { const reason = type === 'by-name' ? 'match-by-name' : 'match-by-location'; - const options = - type === 'by-name' ? undefined : { checkIsWithin: true, ignoreNonLiteralNameDiff: true }; + const options = type === 'by-name' ? matchByNameOptions : matchByLocationOptions; const results: MatchResultType[] = []; const unmatchedT: ChildNodeType[] = tList.filter((t) => { @@ -236,15 +277,17 @@ const ContextMatch = (): ContextMatchAlgorithm => { return { unmatchedT, results }; }; + const matchByContextOptions: MatchOptions = { + acceptLocalNameMatch: true, + ignoreNonLiteralNameDiff: true, + }; const matchByContext = ( tList: ChildNodeType[], aList: ChildNodeType[] ): MatchMethodResult => { const results: MatchResultType[] = []; if (tList.length === aList.length) { - const hasMismatch = tList.find( - (t, idx) => !aList[idx].match(t, { ignoreNonLiteralNameDiff: true }) - ); + const hasMismatch = tList.find((t, idx) => !aList[idx].match(t, matchByContextOptions)); if (!hasMismatch) { tList.forEach((t, idx) => results.push([t, [aList[idx]], 'match-by-context'])); return { unmatchedT: [], results }; @@ -304,14 +347,12 @@ const ContextMatch = (): ContextMatchAlgorithm => { const matchResults: TestResult[] = []; matchChildren('data', tContainer, aContainer, (result) => - matchResults.push(...handleTestBlockMatch(result, false)) + matchResults.push(...handleTestBlockMatch(result)) ); matchChildren('container', tContainer, aContainer, (result) => matchResults.push(...handleDescribeBlockMatch(result)) ); - aContainer.group.forEach((c) => matchResults.push(...matchContainers(tContainer, c))); - return matchResults; }; @@ -338,7 +379,7 @@ const ContextMatch = (): ContextMatchAlgorithm => { return {}; } - let aList = aContainer.unmatchedNodes(type, { ungroup: true }); + let aList = aContainer.unmatchedNodes(type); const matched = (['by-name', 'by-location'] as ClassicMatchType[]).flatMap((matchType) => { const matchResult = classicMatch(matchType, tList, aList); tList = matchResult.unmatchedT; @@ -357,10 +398,10 @@ const ContextMatch = (): ContextMatchAlgorithm => { // handle unmatched container nodes const cFallback = doMatch('container', (r) => { const [t] = r; - return t.isMatched ? [] : handleDescribeBlockMatch(r); + return t.isMatched ? [] : handleDescribeBlockMatch(r, false); }); // handle unmatched data nodes - const dFallback = doMatch('data', (r) => handleTestBlockMatch(r, false)); + const dFallback = doMatch('data', (r) => handleTestBlockMatch(r, false, false)); return { ...dFallback, @@ -369,7 +410,7 @@ const ContextMatch = (): ContextMatchAlgorithm => { }; const toUnmatchedResults = (nodes: DataNode[]): TestResult[] => - nodes.flatMap((t) => handleTestBlockMatch([t, [], 'match-failed'], true)); + nodes.flatMap((t) => handleTestBlockMatch([t, [], 'match-failed'], true, false)); const match = ( tContainer: ContainerNode, @@ -413,11 +454,11 @@ const { match } = ContextMatch(); export const matchTestAssertions = ( fileName: string, sourceRoot: ParsedNode, - assertions: TestAssertionStatus[], + assertions: TestAssertionStatus[] | ContainerNode, verbose = false ): TestResult[] => { const tContainer = buildSourceContainer(sourceRoot); - const aContainer = buildAssertionContainer(assertions); + const aContainer = Array.isArray(assertions) ? buildAssertionContainer(assertions) : assertions; const messaging = createMessaging(fileName, verbose); return match(tContainer, aContainer, messaging); diff --git a/src/TestResults/match-node.ts b/src/TestResults/match-node.ts index f08faad48..c8213ce7b 100644 --- a/src/TestResults/match-node.ts +++ b/src/TestResults/match-node.ts @@ -2,6 +2,8 @@ * internal classes used by `match-by-context` */ +import { ParsedRange } from 'jest-editor-support'; + type IsGroupType = 'yes' | 'no' | 'maybe'; const IsMatchedEvents = ['match-by-context', 'match-by-name', 'match-by-location'] as const; @@ -19,6 +21,10 @@ export type MatchEvent = export interface MatchOptions { /** if true, will ignore name difference if both nodes have NonLiteral names */ ignoreNonLiteralNameDiff?: boolean; + + // accept regular name, i.e. the name of the node, not the fullName, match. + acceptLocalNameMatch?: boolean; + /** if true, will ignore isGroupNode() difference */ ignoreGroupDiff?: boolean; /** if true, will perform position check to see if "this" is enclosed within "other" node */ @@ -49,8 +55,8 @@ export interface OptionalAttributes { fullName?: string; isGroup?: IsGroupType; nonLiteralName?: boolean; - // zero-based line range - range?: { start: number; end: number }; + // zero-based location range + range?: ParsedRange; } export class BaseNode { name: string; @@ -123,8 +129,8 @@ export class BaseNode { return false; } return ( - this.attrs.range.start <= another.attrs.range.start && - this.attrs.range.end >= another.attrs.range.end + this.attrs.range.start.line <= another.attrs.range.start.line && + this.attrs.range.end.line >= another.attrs.range.end.line ); } @@ -181,21 +187,31 @@ export class BaseNode { **/ match(other: BaseNode, options?: MatchOptions): boolean { - const posNotMatch = options?.checkIsWithin && !other.contains(this); - const matchedAsNonLiteralName = + // check position + if (options?.checkIsWithin && !other.contains(this)) { + return false; + } + // check name + const ignoreNameDiff = options?.ignoreNonLiteralNameDiff && this.maybeNonLiteralName() && other.maybeNonLiteralName(); - const nameNotMatch = this.fullName !== other.fullName && !matchedAsNonLiteralName; - const groupNotMatch = - !options?.ignoreGroupDiff && - !( - this.isGroupNode() === 'maybe' || - other.isGroupNode() === 'maybe' || - this.isGroupNode() === other.isGroupNode() - ); - - return !(posNotMatch || nameNotMatch || groupNotMatch); + if ( + this.fullName !== other.fullName && + !ignoreNameDiff && + !(options?.acceptLocalNameMatch && this.name === other.name) + ) { + return false; + } + + // check group + const ignoreGroupDiff = + options?.ignoreGroupDiff || this.isGroupNode() === 'maybe' || other.isGroupNode() === 'maybe'; + if (this.isGroupNode() !== other.isGroupNode() && !ignoreGroupDiff) { + return false; + } + + return true; } /** @@ -230,9 +246,6 @@ export class DataNode extends BaseNode { } } -export interface UnmatchedOptions { - ungroup?: boolean; -} export type ContextType = 'container' | 'data'; export class ContainerNode extends BaseNode { @@ -307,6 +320,12 @@ export class ContainerNode extends BaseNode { .map((nodes) => nodes.find((n) => !n.hasEvent('invalid-location'))?.zeroBasedLine) .filter((n) => n != null) as number[]; this.zeroBasedLine = topLines.length > 0 ? Math.min(...topLines) : -1; + if (!this.attrs.range) { + this.attrs.range = { + start: { line: this.zeroBasedLine, column: 0 }, + end: { line: this.zeroBasedLine, column: 0 }, + }; + } } } @@ -338,18 +357,10 @@ export class ContainerNode extends BaseNode { } }); } + // extract all unmatched data node - public unmatchedNodes = ( - type: C, - options?: UnmatchedOptions - ): ChildNodeType[] => { - const unmatched = this.allChildNodes(type).filter((n) => !n.isMatched); - - if (options?.ungroup) { - unmatched.forEach((u) => u.ungroup()); - } - return unmatched; - }; + public unmatchedNodes = (type: C): ChildNodeType[] => + this.allChildNodes(type).filter((n) => !n.isMatched); } export type NodeType = ContainerNode | DataNode; diff --git a/tests/TestResults/match-by-context.test.ts b/tests/TestResults/match-by-context.test.ts index de6cf855b..0e7e682c1 100644 --- a/tests/TestResults/match-by-context.test.ts +++ b/tests/TestResults/match-by-context.test.ts @@ -453,6 +453,30 @@ describe('matchTestAssertions', () => { ]) ); }); + it('deeper it.each within describe.each', () => { + const t1 = helper.makeItBlock('test.each $x', [1, 0, 5, 0], { lastProperty: 'each' }); + const d1 = helper.makeDescribeBlock('d-1.each $var', [t1], { lastProperty: 'each' }); + const d2 = helper.makeDescribeBlock('d-2', [d1]); + const t2 = helper.makeItBlock('empty test', [6, 0, 7, 0]); + + const a1 = helper.makeAssertion('`test.each a`', 'KnownSuccess', ['d-1.each 1'], [1, 0]); + const a2 = helper.makeAssertion('test.each b', 'KnownFail', ['d-1.each 1'], [1, 0]); + const a3 = helper.makeAssertion('test.each a', 'KnownSuccess', ['d-1.each 2'], [1, 0]); + const a4 = helper.makeAssertion('test.each b', 'KnownSuccess', ['d-1.each 2'], [1, 0]); + + const sourceRoot = helper.makeRoot([d2, t2]); + const matched = match.matchTestAssertions('a file', sourceRoot, [a1, a2, a3, a4]); + // expect(matched).toHaveLength(5); + expect(matched.map((m) => [m.name, m.start.line, m.status, reason(m)])).toEqual( + expect.arrayContaining([ + [a1.fullName, t1.start.line - 1, a1.status, 'match-by-location'], + [a2.fullName, t1.start.line - 1, a2.status, 'match-by-location'], + [a3.fullName, t1.start.line - 1, a3.status, 'match-by-location'], + [a4.fullName, t1.start.line - 1, a4.status, 'match-by-location'], + [t2.name, t2.start.line - 1, 'Unknown', 'match-failed'], + ]) + ); + }); }); }); it('test name precedence: assertion.fullName > assertion.title > testSource.name', () => { @@ -848,4 +872,45 @@ describe('matchTestAssertions', () => { ]); }); }); + it('matched assertions would be updated with source range', () => { + const getRange = (t) => ({ + start: { line: t.start.line - 1, column: t.start.column - 1 }, + end: { line: t.end.line - 1, column: t.end.column - 1 }, + }); + const t1 = helper.makeItBlock('a test $seq', [2, 5, 6, 51], { lastProperty: 'each' }); + const d1 = helper.makeDescribeBlock('desc-1', [t1], { + start: { line: 1, column: 3 }, + end: { line: 7, column: 3 }, + }); + + const a1 = helper.makeAssertion('a test 1', 'KnownSuccess', ['desc-1'], [3, 0]); + const a2 = helper.makeAssertion('a test 2 ', 'KnownFail', ['desc-1'], [3, 0]); + + const sourceRoot = helper.makeRoot([d1]); + const assertionRoot = match.buildAssertionContainer([a1, a2]); + const matched = match.matchTestAssertions('a file', sourceRoot, assertionRoot); + + expect(matched.map((m) => toTestResultRecord(m))).toMatchTestResults([ + [a1.fullName, t1.start.line - 1, a1.status, ['match-by-context']], + [a2.fullName, t1.start.line - 1, a2.status, ['match-by-context']], + ]); + const assertionDescribe = assertionRoot.childContainers[0]; + expect(assertionDescribe.attrs.range).toEqual(getRange(d1)); + expect(assertionDescribe.attrs.range).toEqual(getRange(d1)); + assertionDescribe.childData.forEach((c) => expect(c.attrs.range).toEqual(getRange(t1))); + }); + + // see https://github.com/jest-community/vscode-jest/issues/608#issuecomment-849770258 + it('dynamic named describe block should work for match-by-context', () => { + const t1 = helper.makeItBlock('simple test', [1, 0, 6, 0]); + const d1 = helper.makeDescribeBlock('`with ${TemplateLiteral}`', [t1]); + + const a1 = helper.makeAssertion('simple test', 'KnownSuccess', ['with whatever'], [1, 0]); + const sourceRoot = helper.makeRoot([d1]); + const matched = match.matchTestAssertions('a file', sourceRoot, [a1]); + + expect(matched.map((m) => toTestResultRecord(m))).toMatchTestResults([ + [a1.fullName, t1.start.line - 1, a1.status, ['match-by-context']], + ]); + }); }); diff --git a/tests/TestResults/match-node.test.ts b/tests/TestResults/match-node.test.ts index 7f052ce27..075a7d223 100644 --- a/tests/TestResults/match-node.test.ts +++ b/tests/TestResults/match-node.test.ts @@ -43,8 +43,14 @@ describe('BaseNode', () => { `( 'check location: $loc1 isWithin $loc2? $shouldMatch', ({ loc1, loc2, options, shouldMatch }) => { - const range1 = loc1 && { start: loc1[0], end: loc1[1] }; - const range2 = loc2 && { start: loc2[0], end: loc2[1] }; + const range1 = loc1 && { + start: { line: loc1[0], column: 0 }, + end: { line: loc1[1], column: 0 }, + }; + const range2 = loc2 && { + start: { line: loc2[0], column: 0 }, + end: { line: loc2[1], column: 0 }, + }; const n1 = new BaseNode('n1', 10, { fullName: 'n1', range: range1,