diff --git a/CHANGELOG.md b/CHANGELOG.md index 517805ff63..fde26e979e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In a repository that holds more than one Shopify theme, a theme's `{% render %}`, `{% include %}` and `{% section %}` tags and the sections its JSON templates list now link only to that theme's own snippets and sections, which is where Shopify looks for them. Before, a section or snippet the theme doesn't have was linked to another theme's file of the same name, and a theme kept in a subfolder could link to the root theme's copy instead of its own. A reference the theme can't satisfy now links to nothing. Projects with a single theme are indexed as before. Re-index projects with several Shopify themes after upgrading. - `codegraph sync` and the file watcher now link a Shopify section or snippet to the files that name it when it is added after them, or comes back after being deleted, whether they name it in a `{% render %}`, `{% include %}` or `{% section %}` tag or as a section `type` in a JSON template. Before, the link appeared only once a file naming it changed or the project was re-indexed, even in a repository with a single theme, so the new section or snippet looked unused and `codegraph affected` missed the files that use it. Re-index Shopify themes after upgrading to pick up links an earlier version missed. - `codegraph sync` and the file watcher now link a route to the page or layout it loads lazily when that file is added after the router was indexed, comes back after being deleted, or gets the component the route renders in a later edit, whether or not the import names the file's extension: React Router's `lazy: () => import('./pages/Team')`, Vue Router's `component: () => import('@/views/Login')` and Angular's `loadComponent: () => import('./home/home.component')`. Before, the route stayed unlinked until the router file changed or the project was re-indexed, so the page looked unused and `codegraph affected` missed the tests that reach it through the router. Re-index after upgrading to pick up links an earlier version missed. +- A file named the way Google names its tests, like `wire_format_unittest.cc` in protocolbuffers/protobuf, `logging_unittest.cc` in google/glog or a Python `foo_unittest.py`, now counts as a test, as `wire_format_test.cc` already did. Before, it was taken for production code: search and `codegraph_explore` ranked it alongside the code it tests, `codegraph affected` never listed it, and calls in production code could link to a class or type a unittest declares for itself, as google/breakpad's `stack_frame_entries_.size()` did to a unittest's `StackHelper`. Re-index projects that have such files after upgrading. - In C, C++ and Objective-C, a struct or class member declared as a pointer, reference, array or function, like `SharedState* shared;`, `jv elements[];` or `virtual Status Put(…) = 0;`, no longer makes the struct look like it inherits from the member's type. Before, `codegraph_explore` and impact listed those types as base classes, every struct holding a pointer to a type as one of its subclasses, and a method of that struct as an override of the type's method with the same name. Real base classes like `class Derived : public Base`, Objective-C superclasses and Go's embedded fields are unchanged. Re-index C, C++ and Objective-C projects after upgrading. - In C++, a call on a variable, parameter or member declared through a `typedef` or `using` alias now links to the method of the type the alias names, with the alias looked up the way C++ does it: in the calling function, its class and the classes that class inherits from, then the namespaces around it. In google/leveldb, `table_.Insert(buf)` on a `Table table_;` declared next to `typedef SkipList Table;` now reaches `SkipList::Insert` instead of the unrelated `HandleTable::Insert`, and `Table::Iterator iter(&table_);` reaches `SkipList::Iterator`'s constructor. A class that only shares the alias's name is no longer taken for it, and when the aliased type has no such method, as with an alias of `std::vector` or of a template parameter, no method is guessed from the receiver's name. Re-index C++ projects after upgrading. - In C++, a base class like `Message` in `class DynamicMessage final : public Message` now links to the class C++ itself would pick: the one declared in the namespaces around the class, or brought in by the file's `using` declarations and namespace aliases, and the class a `typedef` or `using` alias names. Before, a base class was matched by its name alone, so it could land on a class of the same name in another namespace or another language: in protocolbuffers/protobuf, `DynamicMessage` derived from the JSON parser's internal `Message` class, and in google/leveldb and RocksDB, iterators derived from an unrelated nested `Iterator` class such as the skip list's. The wrong base showed up in `codegraph_explore`'s type hierarchy and in impact, and tied the base's methods to the wrong overrides. A template parameter used as a base no longer links to a class that shares its name, and a base written from the global scope, like `public ::testing::Test`, now links to its class. Re-index C++ projects after upgrading. diff --git a/__tests__/cli-affected-test-conventions.test.ts b/__tests__/cli-affected-test-conventions.test.ts index 1887e934f6..7418fde255 100644 --- a/__tests__/cli-affected-test-conventions.test.ts +++ b/__tests__/cli-affected-test-conventions.test.ts @@ -41,10 +41,13 @@ describe('codegraph affected — test-file conventions (#1507)', () => { w('pkg/test_calc.py', 'from pkg.calc import add\n\ndef test_add():\n assert add(1, 2) == 3\n'); w('src/main/kotlin/app/Calc.kt', 'package app\n\nclass Calc {\n fun add(a: Int, b: Int): Int = a + b\n}\n'); w('src/test/kotlin/app/CalcTest.kt', 'package app\n\nclass CalcTest {\n fun addsNumbers() { Calc().add(1, 2) }\n}\n'); + w('src/wire_format.h', '#pragma once\n\nint ByteSize(int value);\n'); + w('src/wire_format.cc', '#include "wire_format.h"\n\nint ByteSize(int value) { return value; }\n'); + w('src/wire_format_unittest.cc', '#include "wire_format.h"\n\nvoid ComputesByteSize() { ByteSize(1); }\n'); const cg = CodeGraph.initSync(dir); await cg.indexAll(); cg.close(); - }); + }, 60_000); afterAll(() => { fs.rmSync(dir, { recursive: true, force: true }); @@ -59,6 +62,10 @@ describe('codegraph affected — test-file conventions (#1507)', () => { expect(affected(dir, ['src/main/kotlin/app/Calc.kt'])).toEqual(['src/test/kotlin/app/CalcTest.kt']); }); + it("reports Google's C++ _unittest.cc beside the code it tests", () => { + expect(affected(dir, ['src/wire_format.h', 'src/wire_format.cc'])).toEqual(['src/wire_format_unittest.cc']); + }); + it('still honours an explicit --filter glob', () => { expect(affected(dir, ['math.go', '--filter', '*_test.go'])).toEqual(['math_test.go']); expect(affected(dir, ['math.go', '--filter', '*.spec.ts'])).toEqual([]); diff --git a/__tests__/is-test-file.test.ts b/__tests__/is-test-file.test.ts index a24e48962a..08b67e7d48 100644 --- a/__tests__/is-test-file.test.ts +++ b/__tests__/is-test-file.test.ts @@ -10,7 +10,7 @@ * `manifest.kt` / a `RealCall.kt` production file must NOT be flagged. */ import { describe, it, expect } from 'vitest'; -import { isTestFile } from '../src/search/query-utils'; +import { isTestFile, isTestPath } from '../src/search/query-utils'; describe('isTestFile', () => { it('flags test-support modules and doubles by directory name', () => { @@ -56,6 +56,30 @@ describe('isTestFile', () => { expect(isTestFile('project/tests/foo.rb')).toBe(true); }); + it("flags Google's unittest-named files as tests", () => { + // protobuf's 62 and Chromium's: `wire_format_unittest.cc` is a suite, not + // the code it tests, even with no `test/` directory above it. + for (const suite of [ + 'src/google/protobuf/wire_format_unittest.cc', + 'src/google/protobuf/wire_format_unittest.h', + 'src/compiler/register-allocator-unittest.cpp', + 'lib/parser.unittest.js', + 'tools/run_all_unittests.cc', + 'build/android/pylib/device_unittest.py', + ]) { + expect(isTestPath(suite), suite).toBe(true); + expect(isTestFile(suite), suite).toBe(true); + } + }); + + it('does NOT flag a file merely named unittest', () => { + // promtool's `unittest.go` is the code that runs rule tests, and + // CPython's `unittest` package is the framework itself. + expect(isTestPath('cmd/promtool/unittest.go')).toBe(false); + expect(isTestPath('Lib/unittest/case.py')).toBe(false); + expect(isTestFile('cmd/promtool/unittest.go')).toBe(false); + }); + it('does NOT flag production files that merely contain "test" lowercase', () => { // The fix is capital-led so camelCase boundaries distinguish these. expect(isTestFile('src/latest/loader.kt')).toBe(false); diff --git a/__tests__/unittest-named-files.test.ts b/__tests__/unittest-named-files.test.ts new file mode 100644 index 0000000000..69baff7d28 --- /dev/null +++ b/__tests__/unittest-named-files.test.ts @@ -0,0 +1,150 @@ +/** + * Google names its tests `foo_unittest.cc` (protobuf, Breakpad, glog, + * Chromium) and Chromium names its Python tests `foo_unittest.py`, where + * every other convention puts a separator right before `test`. The test-file + * check wanted that separator, so each of them read as production code and the + * resolver's test rules ran backwards for it. A unittest could not reach the + * test helpers it uses (`device.reboot()` on a fake from `tests/`), while + * production code resolved into a unittest's own declarations: Breakpad's + * `stack_frame_entries_.size()` went to a unittest's `StackHelper::size`, and + * a template's `AddressType()` to the `typedef` one unittest declares. + */ +import { describe, it, expect, afterAll, beforeAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; + +let root = ''; +let cg: CodeGraph; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-unittest-named-')); + const files: Record = { + // A test helper in a test suite, and a unittest that uses it. + 'tests/__init__.py': '', + 'tests/fake_device.py': `class FakeDevice: + def reboot(self): + return True +`, + 'tools/device/device_unittest.py': `from tests.fake_device import FakeDevice + + +def test_reboot(): + device = FakeDevice() + return device.reboot() +`, + // The same in C++: protobuf's TestUtil, reached through the header. + 'src/message.h': `#ifndef MESSAGE_H_ +#define MESSAGE_H_ + +namespace protobuf { + +class Message { + public: + void Clear(); +}; + +} // namespace protobuf + +#endif +`, + 'src/message.cc': `#include "message.h" + +namespace protobuf { + +void Message::Clear() {} + +} // namespace protobuf +`, + 'src/test_util.h': `#ifndef TEST_UTIL_H_ +#define TEST_UTIL_H_ + +#include "message.h" + +namespace protobuf { +namespace TestUtil { + +inline void SetAllFields(Message* message) { message->Clear(); } + +} // namespace TestUtil +} // namespace protobuf + +#endif +`, + 'src/wire_format_unittest.cc': `#include "message.h" +#include "test_util.h" + +namespace protobuf { + +void ParsesAllFields() { + Message message; + TestUtil::SetAllFields(&message); +} + +} // namespace protobuf +`, + // Production code whose calls name nothing in the project. + 'src/module.cc': `#include + +using std::vector; + +class Module { + public: + int CountEntries() const { return static_cast(stack_frame_entries_.size()); } + + private: + vector stack_frame_entries_; +}; +`, + 'src/range_map-inl.h': `template +bool StoreRange(const AddressType& base) { + AddressType high = AddressType(); + return base < high; +} +`, + // A unittest's own declarations, named like what production code calls. + 'src/ptrace_dumper_unittest.cc': `class StackHelper { + public: + unsigned size() const { return 0; } +}; + +unsigned Depth() { + StackHelper helper; + return helper.size(); +} +`, + 'src/address_map_unittest.cc': `typedef int AddressType; + +AddressType Lookup() { return 0; } +`, + }; + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } + cg = await CodeGraph.init(root, { index: true }); +}, 60_000); + +afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +const edgesFrom = (file: string) => { + const ids = cg.getNodesInFile(file).map((n) => n.id); + return cg.getOutgoingEdgesFrom(ids).filter((e) => e.kind !== 'contains').map((e) => cg.getNode(e.target)!); +}; + +describe("Google's unittest files are tests", () => { + it('a unittest reaches the test helpers it uses', () => { + expect(edgesFrom('tools/device/device_unittest.py').map((n) => n.qualifiedName)).toContain('FakeDevice::reboot'); + expect(edgesFrom('src/wire_format_unittest.cc').map((n) => n.qualifiedName)).toContain('protobuf::TestUtil::SetAllFields'); + }); + + it("production code never resolves into a unittest's own declarations", () => { + for (const file of ['src/module.cc', 'src/range_map-inl.h']) { + expect(edgesFrom(file).map((n) => n.filePath).filter((f) => f.endsWith('_unittest.cc')), file).toEqual([]); + } + }); +}); diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index 6884777790..46dd84752d 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -1549,9 +1549,10 @@ const JVM_TYPE_KINDS: ReadonlySet = new Set(['class', 'interface', 'enum /** * A test suite — a test source set, a `tests/` / `__tests__/` / `spec/` - * directory, a `FooTest.kt` / `test_foo.py` / `foo.test.ts` file — as opposed - * to test-support code a project ships (`testing/`, `fakes/`, a `*-test` - * module like kotlinx-coroutines-test), which its own code may use. + * directory, a `FooTest.kt` / `test_foo.py` / `foo.test.ts` / `foo_unittest.cc` + * file — as opposed to test-support code a project ships (`testing/`, + * `fakes/`, a `*-test` module like kotlinx-coroutines-test), which its own + * code may use. */ function isTestSuitePath(filePath: string): boolean { if (!isTestPath(filePath)) return false; @@ -1559,7 +1560,7 @@ function isTestSuitePath(filePath: string): boolean { const name = lower.slice(lower.lastIndexOf('/') + 1); const original = filePath.slice(filePath.lastIndexOf('/') + 1); // (`…Spec.java` alone is no test: halo's `IndexSpecs`, okhttp's `ConnectionSpec`.) - if (name.startsWith('test_') || /[._-](?:test|tests)\.[a-z0-9]+$|[._](?:spec|specs)\.[a-z0-9]+$/.test(name) || + if (name.startsWith('test_') || /[._-](?:test|tests|unittest|unittests)\.[a-z0-9]+$|[._](?:spec|specs)\.[a-z0-9]+$/.test(name) || // CamelCase suffixes where the language names tests so: not `useTests.ts`, a React hook. /(?:Test|Tests|TestCase)\.(?:java|kt|kts|swift|cs|scala|groovy|m|mm|vb|fs)$/.test(original) || name === 'conftest.py') return true; return /(?:^|\/)(?:tests?|__tests__|specs?|e2e)\//.test(lower) || /(?:^|\/)[A-Za-z0-9]*(?:Test|Tests|Spec)\//.test(filePath); diff --git a/src/search/query-utils.ts b/src/search/query-utils.ts index 03817b8c5f..e51cbe5d46 100644 --- a/src/search/query-utils.ts +++ b/src/search/query-utils.ts @@ -317,8 +317,10 @@ export function isTestPath(filePath: string): boolean { if ( lowerName.startsWith('test_') || // python: test_foo.py lowerName.startsWith('test.') || - // separator-delimited: foo_test.go, foo.test.ts, foo-spec.rb, bar_spec.py - /[._-](test|tests|spec|specs)\.[a-z0-9]+$/.test(lowerName) || + // separator-delimited: foo_test.go, foo.test.ts, foo-spec.rb, bar_spec.py, + // and Google's C++ foo_unittest.cc (protobuf, Chromium). A bare + // `unittest.go` is no test: promtool's is the code that runs rule tests. + /[._-](test|tests|unittest|unittests|spec|specs)\.[a-z0-9]+$/.test(lowerName) || // CamelCase suffix (Java/Kotlin/Swift/C#/Scala): FooTest.kt, BarTests.swift, // BazSpec.scala, QuxTestCase.java. Capital-led so "latest.kt"/"manifest.kt" // (lowercase "test") are NOT matched.