mirror of
https://github.com/apache/superset.git
synced 2026-05-19 23:05:14 +00:00
fix(subdirectory): reinstate invariants scan, harden scanner
The shard-6 hang reproduced on master independently of this PR — earlier diagnostic edits (skipping the invariants scan) are no longer needed. Reinstates the static-invariant scan in `navigationUtils.invariants.test.ts` and keeps the previously seeded PATH_UTILS_IMPORT_ALLOWLIST so the suite is GREEN today and shrinks as each migration commit lands. Also hoists the regex compile out of the per-line loop in `scanSource`. With no `g` flag, `RegExp.exec()` ignores `lastIndex`, so recompiling per line was wasted allocation across ~1.5M lines workspace- wide. If the source pattern includes `g`, the helper now strips it once at the top of the file rather than relying on per-line construction. Adds `jest.setTimeout(20000)` to `navigationUtils.test.ts` as a defence-in-depth safety net — any future hang surfaces a Jest timeout error with the test name rather than running for the workflow's six-hour wallclock limit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -173,11 +173,15 @@ export function scanSource(options: ScanOptions): ScanHit[] {
|
||||
const contents = readFileSync(absoluteFile, 'utf8');
|
||||
const lines = contents.split('\n');
|
||||
|
||||
// Reuse a single regex per file. Without the `g` flag, RegExp's
|
||||
// `lastIndex` is ignored on `.exec()` — recompiling per-line was
|
||||
// wasted allocation across ~1.5M lines workspace-wide.
|
||||
const lineRegex = pattern.flags.includes('g')
|
||||
? new RegExp(pattern.source, pattern.flags.replace('g', ''))
|
||||
: pattern;
|
||||
|
||||
for (let index = 0; index < lines.length; index += 1) {
|
||||
const lineText = lines[index];
|
||||
// Re-create the regex per line so the global flag's lastIndex doesn't
|
||||
// bleed across iterations.
|
||||
const lineRegex = new RegExp(pattern.source, pattern.flags);
|
||||
const match = lineRegex.exec(lineText);
|
||||
if (match) {
|
||||
hits.push({
|
||||
|
||||
@@ -16,8 +16,7 @@
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
// `scanSource` and `expectNoHits` are imported by the active scan when it is
|
||||
// reinstated (see comment block below).
|
||||
import { expectNoHits, scanSource } from 'spec/helpers/sourceTreeScanner';
|
||||
|
||||
// =============================================================================
|
||||
// Layer 2 example: structural invariant
|
||||
@@ -65,23 +64,35 @@ const PATH_UTILS_IMPORT_ALLOWLIST: string[] = [
|
||||
'src/views/CRUD/hooks.ts',
|
||||
];
|
||||
|
||||
// The full scan (`no file outside navigationUtils.ts imports ensureAppRoot
|
||||
// or makeUrl from pathUtils`) is temporarily removed while the CI shard-hang
|
||||
// root cause is being isolated — the scanner walks ~1591 source files and a
|
||||
// recent shard-6 run hung for 3+ hours without logging PASS for any of our
|
||||
// new test files. We restore the scan in a follow-up commit once we know
|
||||
// whether `scanSource` was the cause (we suspect per-line `new RegExp(...)`
|
||||
// allocation under sync recursion). Keeping the file present with a sentinel
|
||||
// so the import path stays valid for the helpers + future scans.
|
||||
//
|
||||
// The static-invariant pattern is fully implemented in `sourceTreeScanner.ts`
|
||||
// and `scanSource` is exported for re-use. Reinstatement plan:
|
||||
// 1. Hoist the regex compile out of the per-line loop in scanSource
|
||||
// 2. Add a per-test timing log so any future hang surfaces a culprit
|
||||
// 3. Re-add the scan with the seeded PATH_UTILS_IMPORT_ALLOWLIST
|
||||
test('PATH_UTILS_IMPORT_ALLOWLIST entries are workspace-relative paths', () => {
|
||||
for (const entry of PATH_UTILS_IMPORT_ALLOWLIST) {
|
||||
expect(entry.startsWith('/')).toBe(false);
|
||||
expect(entry.includes('\\')).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
test('no file outside navigationUtils.ts imports ensureAppRoot or makeUrl from pathUtils', () => {
|
||||
const hits = scanSource({
|
||||
pattern: /\b(?:ensureAppRoot|makeUrl)\b/,
|
||||
allowlist: [
|
||||
// The two modules that are *allowed* to know about path prefixing.
|
||||
// `pathUtils.ts` defines the helpers; `navigationUtils.ts` is the only
|
||||
// re-export sanctioned for the rest of the codebase to consume.
|
||||
'src/utils/pathUtils.ts',
|
||||
'src/utils/navigationUtils.ts',
|
||||
// SupersetClient has its own `appRoot` configuration path — it does
|
||||
// not import from `pathUtils`. Excluded so a future occurrence of
|
||||
// the word `appRoot` in connection internals does not trip this scan.
|
||||
'packages/superset-ui-core/src/connection/SupersetClientClass.ts',
|
||||
'packages/superset-ui-core/src/connection/normalizeBackendUrls.ts',
|
||||
...PATH_UTILS_IMPORT_ALLOWLIST,
|
||||
],
|
||||
});
|
||||
|
||||
expectNoHits(
|
||||
hits,
|
||||
'Found imports of ensureAppRoot / makeUrl outside navigationUtils.ts. ' +
|
||||
'Use the focused helpers (openInNewTab, redirect, getShareableUrl, AppLink) ' +
|
||||
'instead, or add the file to PATH_UTILS_IMPORT_ALLOWLIST with justification.',
|
||||
);
|
||||
});
|
||||
|
||||
@@ -18,6 +18,10 @@
|
||||
*/
|
||||
import { withApplicationRoot } from 'spec/helpers/withApplicationRoot';
|
||||
|
||||
// Failsafe so a future hang surfaces a Jest timeout error with the test name
|
||||
// rather than running for the workflow's 6-hour wallclock limit.
|
||||
jest.setTimeout(20000);
|
||||
|
||||
// =============================================================================
|
||||
// Layer 1 example: openInNewTab
|
||||
// =============================================================================
|
||||
|
||||
Reference in New Issue
Block a user