More tests for the ConformancePatterns, and fix substr-based confusion
Adding a bunch more tests around various more complex cases (shadowing, function parameters, and whitespace) revealed a bug where I mixed start/end and start/length patterns. So I'm taking the opportunity to switch to String.prototype.slice for the matcher, which is slightly less surprising in its behavior.
PiperOrigin-RevId: 250134904
diff --git a/internal/tsetse/tests/ban_conformance_pattern/property_write_test.ts b/internal/tsetse/tests/ban_conformance_pattern/property_write_test.ts
index 997577e..42290f2 100644
--- a/internal/tsetse/tests/ban_conformance_pattern/property_write_test.ts
+++ b/internal/tsetse/tests/ban_conformance_pattern/property_write_test.ts
@@ -3,19 +3,63 @@
import {compileAndCheck, customMatchers} from '../../util/testing/test_support';
describe('BANNED_PROPERTY_WRITE', () => {
+ const rule = new ConformancePatternRule({
+ errorMessage: 'do not cite',
+ kind: PatternKind.BANNED_PROPERTY_WRITE,
+ values: ['HTMLQuoteElement.prototype.cite']
+ });
+
it('matches simple examples', () => {
- const source = `const q = document.createElement('q');\n` +
- `q.cite = 'some example string';\n`;
- const rule = new ConformancePatternRule({
- errorMessage: 'do not cite',
- kind: PatternKind.BANNED_PROPERTY_WRITE,
- values: ['HTMLQuoteElement.prototype.cite']
- });
+ const source = [
+ `const q = document.createElement('q');`,
+ `q.cite = 'some example string';`,
+ ].join('\n');
const results = compileAndCheck(rule, source);
expect(results.length).toBe(1);
- expect(results[0])
- .toBeFailureMatching({start: 39, end: 69, errorMessage: 'do not cite'});
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `q.cite = 'some example string'`,
+ errorMessage: 'do not cite'
+ });
+ });
+
+ it('matches precisely, even with whitespace or comments', () => {
+ const source = [
+ `const q = document.createElement('q');`,
+ ` q.cite = 'exampleA';`,
+ `q.cite = 'exampleB' ;`,
+ `/* test1 */ q.cite = /* test2 */ 'exampleC' /* test3 */;`,
+ ].join('\n');
+ const results = compileAndCheck(rule, source);
+
+ expect(results.length).toBe(3);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `q.cite = 'exampleA'`,
+ errorMessage: 'do not cite'
+ });
+ expect(results[1]).toBeFailureMatching({
+ matchedCode: `q.cite = 'exampleB'`,
+ errorMessage: 'do not cite'
+ });
+ expect(results[2]).toBeFailureMatching({
+ matchedCode: `q.cite = /* test2 */ 'exampleC'`,
+ errorMessage: 'do not cite'
+ });
+ })
+
+ it('understands function prototypes', () => {
+ const source = [
+ `function foo(q:HTMLQuoteElement) {`,
+ ` q.cite = 'some example string';`,
+ `}`,
+ ].join('\n');
+ const results = compileAndCheck(rule, source);
+
+ expect(results.length).toBe(1);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `q.cite = 'some example string'`,
+ errorMessage: 'do not cite'
+ });
});
it('understands imported symbols', () => {
@@ -23,16 +67,11 @@
`const q = document.createElement('q'); export {q};`,
`import {q} from './file_0'; q.cite = window.name;`
];
- const rule = new ConformancePatternRule({
- errorMessage: 'do not cite',
- kind: PatternKind.BANNED_PROPERTY_WRITE,
- values: ['HTMLQuoteElement.prototype.cite']
- });
const results = compileAndCheck(rule, ...sources);
expect(results.length).toBe(1);
expect(results[0]).toBeFailureMatching({
- matchedCode: 'q.cite = window.name;',
+ matchedCode: 'q.cite = window.name',
fileName: 'file_1.ts',
errorMessage: 'do not cite',
});
@@ -41,16 +80,16 @@
describe('with inheritance', () => {
const source = [
- `class Parent {x:number}`,
+ `class Parent { x: number }`,
`class Child extends Parent {}`,
- `const c:Child = new Child();`,
+ `const c: Child = new Child();`,
`c.x = 1;`,
].join('\n');
// Both of these should have the same results: in `c.x`, `x` matches,
// and `c` is both a Parent and a Child.
const expectedFailure = {
- matchedCode: 'c.x = 1;',
+ matchedCode: 'c.x = 1',
errorMessage: 'found write to x',
};
@@ -76,6 +115,23 @@
expect(r[0]).toBeFailureMatching(expectedFailure);
});
});
+
+ describe('with shadowing', () => {
+ it('does not over-match', () => {
+ const source = [
+ `const q = document.createElement('q');`,
+ `const f1 = (q: {cite: string}) => { q.cite = 'example 1'; };`,
+ ].join('\n');
+ const rule = new ConformancePatternRule({
+ errorMessage: 'do not cite',
+ kind: PatternKind.BANNED_PROPERTY_WRITE,
+ values: ['HTMLQuoteElement.prototype.cite']
+ });
+ const results = compileAndCheck(rule, source);
+
+ expect(results.length).toBe(0);
+ });
+ });
});
beforeEach(() => {
diff --git a/internal/tsetse/util/pattern_engines/property_write_engine.ts b/internal/tsetse/util/pattern_engines/property_write_engine.ts
index 7af7ccf..ea90e60 100644
--- a/internal/tsetse/util/pattern_engines/property_write_engine.ts
+++ b/internal/tsetse/util/pattern_engines/property_write_engine.ts
@@ -38,10 +38,12 @@
!isPropertyWriteExpression(n)) {
return;
}
- debugLog(`inspecting ${n.getFullText().trim()}`);
+ debugLog(`inspecting ${n.getText().trim()}`);
if (this.matcher.matches(n.left, c.typeChecker)) {
const fix: Fix|undefined =
this.fixer ? this.fixer.getFixForFlaggedNode(n) : undefined;
+ debugLog(`Match. Reporting failure (boundaries: ${n.getStart()}, ${
+ n.getEnd()}] on node [${n.getText()}]`);
c.addFailureAtNode(n, this.config.errorMessage, fix);
}
}
diff --git a/internal/tsetse/util/testing/test_support.ts b/internal/tsetse/util/testing/test_support.ts
index f79192c..6177710 100644
--- a/internal/tsetse/util/testing/test_support.ts
+++ b/internal/tsetse/util/testing/test_support.ts
@@ -79,7 +79,7 @@
const actualDiagnostic = actualFailure.toDiagnostic();
let regrets = '';
if (exp === undefined) {
- regrets += 'The rule requires two arguments. ';
+ regrets += 'The matcher requires two arguments. ';
}
if (exp.fileName) {
if (!actualDiagnostic.file) {
@@ -100,12 +100,17 @@
if (!actualDiagnostic.file) {
regrets += `Expected diagnostic to have a source file, but it had ${
actualDiagnostic.file}. `;
+ } else if (!actualDiagnostic.start) {
+ // I don't know how this could happen, but typings say so.
+ regrets += `Expected diagnostic to have a starting position. `;
} else {
- const foundMatchedCode = actualDiagnostic.file.getFullText().substr(
+ const foundMatchedCode = actualDiagnostic.file.getFullText().slice(
Number(actualDiagnostic.start), actualDiagnostic.end);
if (foundMatchedCode != exp.matchedCode) {
regrets += `Expected diagnostic to match ${
- exp.matchedCode}, but was ${foundMatchedCode}`;
+ exp.matchedCode}, but was ${foundMatchedCode} (from ${
+ Number(
+ actualDiagnostic.start)} to ${actualDiagnostic.end}). `;
}
}
}
