feat: cache test class discovery #16
Conversation
WalkthroughAdds disk-backed and in-memory caching for discovered test classes and per-class test-function metadata with signature validation and a force-refresh option; updates discovery flows to consult/persist caches, extends _TestFunction constructor to accept tags and useTransactions, and adds tests and docs for the new options. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as TestRunner
participant Store as ClassStore
participant Disk as CacheFile
participant Suite as TestSuite
participant Class as TestClass
Note over Runner: Request test classes
Runner->>Store: Compute class-store signature
alt signature matches and not forced
Runner->>Disk: Load cached class names & function metadata
Disk-->>Runner: Cached class list + function cache
Runner->>Runner: Populate in-memory caches
else cache absent or signature changed or force refresh
Runner->>Store: Discover classes via _filterTestClasses
Store-->>Runner: New class list
Runner->>Runner: Update _cachedTestClasses and signature
Runner->>Disk: Save cache (class list + per-class functions)
end
Note over Suite,Runner: Test discovery per class
Suite->>Runner: Ask for cached functions for Class
alt cached functions available
Runner-->>Suite: return cached function metadata
Suite->>Class: Validate functions exist and build _TestFunction wrappers (cached tags/useTransactions)
else no cached functions
Suite->>Class: Inspect methods, build _TestFunction wrappers (parse tags/useTransactions from code)
Suite->>Runner: _updateFunctionCache(class, signature, functions)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
testing/Project/Sources/Classes/TestRunner.4dm (3)
132-147: Be resilient to corrupted/legacy cache files.Add shape/type checks before using parsed JSON and ignore bad caches to avoid hard-to-repro errors.
Example tweak:
If ($cacheFile.exists) var $cacheText : Text $cacheText:=$cacheFile.getText("UTF-8") var $cacheObj : Object $cacheObj:=JSON Parse:C1218($cacheText) - If ($cacheObj#Null:C1517) && ($cacheObj.signature=$signature) + If ($cacheObj#Null:C1517) && ($cacheObj.signature=$signature) \ + && ($cacheObj.classes#Null:C1517) \ + && (Value type:C1509($cacheObj.classes)=Is collection:K8:32) var $classes : Collection $classes:=This:C1470._filterTestClasses($classStore; $cacheObj.classes) This:C1470._cachedTestClasses:=$classes This:C1470._classStoreSignature:=$signature End if End if
148-165: Avoid partial writes: write-then-rename for atomic cache updates.Simultaneous test runs or crashes mid-write can leave a truncated JSON file. Write to a temp file and rename to the final path atomically.
10-11: Initialize new properties explicitly in the constructor.Make initial intent clear and prevent accidental reuse across instances.
You can add in Class constructor():
This:C1470._parseTagFilters() +// Explicitly initialize cache state +This:C1470._cachedTestClasses:=Null:C1517 +This:C1470._classStoreSignature:=""testing/Project/Sources/Classes/_TestRunnerTest.4dm (2)
119-144: Strengthen the cache test to confirm disk read (not recompute).Currently it only checks count equality. Also assert the cache file content remains identical between runs to increase confidence the disk path is used.
Example:
// First run should create the cache file var $classes : Collection $classes:=$runner._getTestClasses() $t.assert.isTrue($t; $cacheFile.exists; "Cache file should be created on first run") + var $beforeText : Text + $beforeText:=$cacheFile.getText("UTF-8") // Clear in-memory cache to simulate fresh run $runner._cachedTestClasses:=Null:C1517 $runner._classStoreSignature:="" // Second run should load from disk and return same number of classes var $classes2 : Collection $classes2:=$runner._getTestClasses() $t.assert.areEqual($t; $classes.length; $classes2.length; "Cache should provide same classes on subsequent runs") + var $afterText : Text + $afterText:=$cacheFile.getText("UTF-8") + $t.assert.areEqual($t; $beforeText; $afterText; "Second run should not rewrite cache file")
115-117: Prefer using the public matcher in assertions for suffix checks.Use
_matchesPattern("*Test")to mirror production logic and avoid relying on implicit “@” semantics in tests.- For each ($class; $testClasses) - $t.assert.isTrue($t; $class.name="@Test"; "All discovered classes should end with 'Test', found: "+$class.name) - End for each + For each ($class; $testClasses) + $t.assert.isTrue($t; $runner._matchesPattern($class.name; "*Test"); "All discovered classes should end with 'Test', found: "+$class.name) + End for each
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
testing/Project/Sources/Classes/TestRunner.4dm(2 hunks)testing/Project/Sources/Classes/_TestRunnerTest.4dm(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
testing/Project/Sources/Classes/*Test.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/*Test.4dm: Name 4D test classes with the suffix "Test" for auto-discovery
Define test methods with names starting with "test_" inside test classes
Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Opt a suite out of parallel execution with the annotation "// #parallel: false"
Automatic transactions are enabled by default; disable per test with "// #transaction: false"
When managing transactions manually, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Files:
testing/Project/Sources/Classes/_TestRunnerTest.4dm
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Files:
testing/Project/Sources/Classes/_TestRunnerTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
🧠 Learnings (4)
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Name 4D test classes with the suffix "Test" for auto-discovery
Applied to files:
testing/Project/Sources/Classes/_TestRunnerTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Define test methods with names starting with "test_" inside test classes
Applied to files:
testing/Project/Sources/Classes/_TestRunnerTest.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
🔇 Additional comments (1)
testing/Project/Sources/Classes/_TestRunnerTest.4dm (1)
78-79: Call-site updates to _filterTestClasses look correct.Passing Null for $classNames exercises the default path that derives keys from the store.
Also applies to: 268-269, 312-313, 352-353
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
testing/Project/Sources/Classes/TestRunner.4dm (2)
75-80: Make cache signature deterministic (sort keys before hashing).Key order isn’t guaranteed; unsorted keys can yield unstable signatures and missed cache hits.
var $classNames : Collection $classNames:=OB Keys:C1719($classStore) + // Ensure deterministic order so signature is stable across runs + $classNames.sort() var $signature : Text $signature:=JSON Stringify:C1217($classNames)
103-129: Guard against stale/malformed entries and centralize suffix matching.Avoid Null deref on
$classInfo.superclassand use_matchesPattern("*Test")rather than hardcoding@Test.Function _filterTestClasses($classStore : Object; $classNames : Collection) : Collection var $classes : Collection $classes:=[] var $className : Text var $classInfo : Object If ($classNames=Null:C1517) $classNames:=OB Keys:C1719($classStore) End if For each ($className; $classNames) $classInfo:=$classStore[$className] - // Skip classes without superclass property (malformed classes) - If ($classInfo.superclass=Null:C1517) + // Skip missing or malformed entries + If ($classInfo=Null:C1517) + continue + End if + // Skip classes without superclass property (malformed classes) + If ($classInfo.superclass=Null:C1517) continue End if // Skip Dataclasses for now If ($classInfo.superclass.name="DataClass") continue End if // Test classes end with "Test", e.g. "MyClassTest" - If ($className="@Test") + If (This:C1470._matchesPattern($className; "*Test")) $classes.push($classInfo) End if End for each return $classes
🧹 Nitpick comments (3)
testing/Project/Sources/Classes/_TestFunction.4dm (1)
25-35: Harden optional param handling for tags.If callers accidentally pass a non-Collection (or Null), parsing should kick in.
- If ($paramCount>=6) && ($tags#Null:C1517) + If ($paramCount>=6) && ($tags#Null:C1517) && (Value type:C1509($tags)=Is collection:K8:32) This:C1470.tags:=$tags Else This:C1470.tags:=This:C1470._parseTags($classCode || "") End iftesting/Project/Sources/Classes/_TestRunnerTest.4dm (1)
402-419: Add class-cache refresh assertion.You verify function-cache refresh; consider mirroring this for class discovery to ensure
_getTestClasses()ignores disk/in-memory caches whenforceCacheRefreshis true.I can draft a small test that toggles
forceCacheRefresh, primes the cache, then asserts a recomputation path is taken (e.g., by stubbing_filterTestClassescall count). Want me to add it?testing/Project/Sources/Classes/TestRunner.4dm (1)
179-191: Minor: avoid hidden side-effects when materializing function cache.Calling
_getTestClasses()from_getFunctionCache()can write cache and trigger discovery. Consider lazy-loading only from disk (or initializing to{}) here to keep responsibilities separated.I can provide a patch that removes the
_getTestClasses()call and initializes_functionCachefrom_loadCache(...)or{}.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
testing/Project/Sources/Classes/TestRunner.4dm(2 hunks)testing/Project/Sources/Classes/_TestFunction.4dm(1 hunks)testing/Project/Sources/Classes/_TestRunnerTest.4dm(6 hunks)testing/Project/Sources/Classes/_TestSuite.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Files:
testing/Project/Sources/Classes/_TestFunction.4dmtesting/Project/Sources/Classes/_TestSuite.4dmtesting/Project/Sources/Classes/_TestRunnerTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
testing/Project/Sources/Classes/*Test.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/*Test.4dm: Name 4D test classes with the suffix "Test" for auto-discovery
Define test methods with names starting with "test_" inside test classes
Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Opt a suite out of parallel execution with the annotation "// #parallel: false"
Automatic transactions are enabled by default; disable per test with "// #transaction: false"
When managing transactions manually, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Files:
testing/Project/Sources/Classes/_TestRunnerTest.4dm
🧠 Learnings (4)
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Applied to files:
testing/Project/Sources/Classes/_TestFunction.4dmtesting/Project/Sources/Classes/_TestSuite.4dmtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Automatic transactions are enabled by default; disable per test with "// #transaction: false"
Applied to files:
testing/Project/Sources/Classes/_TestFunction.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Name 4D test classes with the suffix "Test" for auto-discovery
Applied to files:
testing/Project/Sources/Classes/_TestSuite.4dmtesting/Project/Sources/Classes/_TestRunnerTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Define test methods with names starting with "test_" inside test classes
Applied to files:
testing/Project/Sources/Classes/_TestRunnerTest.4dm
🔇 Additional comments (2)
testing/Project/Sources/Classes/_TestFunction.4dm (1)
31-35: Verify “||” coalescing works for Text in 4D.You’re using
($classCode || "")as a coalesce. Confirm this behaves as intended in compiled mode for Text types; otherwise switch to an explicit Null/empty check.Would you like me to patch these sites to a defensive ternary-style check if needed?
testing/Project/Sources/Classes/_TestRunnerTest.4dm (1)
119-144: Nice coverage of class-cache create/load path.Test exercises cache file creation and reuse; resetting in-memory state validates disk persistence.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
testing/Project/Sources/Classes/TestRunner.4dm (5)
75-81: Deterministic cache signature — nice fix.Sorting class names before JSON Stringify stabilizes the signature across runs.
105-136: Robust filtering of test classes.Null guard for stale entries and centralized suffix match via _matchesPattern("*Test") are spot on.
83-91: Honor forceCacheRefresh in the load/return paths.With refresh requested, we should neither load from disk nor return in‑memory cache. Otherwise users get stale discovery results.
Apply:
- // Try to load from disk cache if not already in memory - If (This:C1470._cachedTestClasses=Null:C1517) + // Try to load from disk cache if not already in memory and refresh not forced + If (Not:C34(This:C1470.forceCacheRefresh)) && (This:C1470._cachedTestClasses=Null:C1517) This:C1470._loadCache($signature; $classStore) End if - // If the class store hasn't changed and we have a cache, return it - If ($signature=This:C1470._classStoreSignature) && (This:C1470._cachedTestClasses#Null:C1517) + // If the class store hasn't changed and we have a cache, return it (unless refresh is forced) + If (Not:C34(This:C1470.forceCacheRefresh)) && ($signature=This:C1470._classStoreSignature) && (This:C1470._cachedTestClasses#Null:C1517) return This:C1470._cachedTestClasses End if
185-198: Avoid disk bootstrap of function cache when refresh is forced.Otherwise we repopulate in-memory state from stale on-disk data.
Apply:
Function _getFunctionCache() : Object If (This:C1470._functionCache=Null:C1517) // Ensure cache is loaded from disk if available - If (This:C1470._cachedTestClasses=Null:C1517) - This:C1470._getTestClasses() - Else - This:C1470._loadCache(This:C1470._classStoreSignature; This:C1470._getClassStore()) - End if + If (Not:C34(This:C1470.forceCacheRefresh)) + If (This:C1470._cachedTestClasses=Null:C1517) + This:C1470._getTestClasses() + Else + This:C1470._loadCache(This:C1470._classStoreSignature; This:C1470._getClassStore()) + End if + End if If (This:C1470._functionCache=Null:C1517) This:C1470._functionCache:=New object:C1471 End if End if return This:C1470._functionCache
141-161: Skip disk read when refresh is forced._loadCache should short-circuit on force to prevent stale data from being rehydrated.
Apply:
Function _loadCache($signature : Text; $classStore : Object) + // Honor forced refresh + If (This:C1470.forceCacheRefresh) + return + End if var $cacheFile : 4D:C1709.File $cacheFile:=This:C1470._cacheFile() If ($cacheFile.exists)
🧹 Nitpick comments (1)
testing/Project/Sources/Classes/TestRunner.4dm (1)
180-184: Make refresh flag parsing case-insensitive.Current check misses “TRUE/True”. Normalize before compare.
Apply:
Function _parseCacheOptions() var $params : Object $params:=This:C1470._parseUserParams() - This:C1470.forceCacheRefresh:=($params.refreshCache="1") || ($params.refreshCache="true") + var $refresh : Text + $refresh:=($params.refreshCache#Null:C1517) ? Lowercase($params.refreshCache) : "" + This:C1470.forceCacheRefresh:=($refresh="1") || ($refresh="true")If you pass refreshCache=TRUE today, does discovery still use cached data? After applying, it should force recompute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testing/Project/Sources/Classes/TestRunner.4dm(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Files:
testing/Project/Sources/Classes/TestRunner.4dm
🧠 Learnings (4)
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Name 4D test classes with the suffix "Test" for auto-discovery
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Define test methods with names starting with "test_" inside test classes
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
🔇 Additional comments (8)
testing/Project/Sources/Classes/TestRunner.4dm (8)
10-13: New cache properties look good.Clear naming and types; defaults to Null are fine.
18-22: Constructor wires in cache option parsing correctly.Initialization order is sensible.
93-99: Compute-and-persist path looks correct.Writes updated classes + signature and returns the fresh collection.
137-140: Cache file location is reasonable.DerivedData + JSON filename is fine for non-versioned persistence.
162-179: Save path looks good.Ensures parent folder exists and persists signature, class names, and function cache.
199-206: Per-class signature is simple and effective.mtime + size is a pragmatic choice for invalidation.
207-218: Cached function retrieval honors signatures.Correctly bypasses cache on forced refresh or signature mismatch.
219-230: Function cache update and persistence look correct.Minimal payload (name/tags/useTransactions) is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
testing/Project/Sources/Classes/_TestSuite.4dm (1)
35-41: Honor forceCacheRefresh: bypass cache path.When refresh is forced, skip loading/using the cache here. This was requested earlier and is still missing.
Apply:
- If (This:C1470.testRunner#Null:C1517) - $cachedFunctions:=This:C1470.testRunner._getCachedFunctionsForClass(This:C1470.class) - End if + If (This:C1470.testRunner#Null:C1517) && Not:C34(This:C1470.testRunner.forceCacheRefresh) + $cachedFunctions:=This:C1470.testRunner._getCachedFunctionsForClass(This:C1470.class) + End if
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testing/Project/Sources/Classes/_TestSuite.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Files:
testing/Project/Sources/Classes/_TestSuite.4dm
🧠 Learnings (1)
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Applied to files:
testing/Project/Sources/Classes/_TestSuite.4dm
🔇 Additional comments (3)
testing/Project/Sources/Classes/_TestSuite.4dm (3)
62-71: Good: single read of class code + unfiltered accumulator for caching.
72-90: Correct split: cache all discovered wrappers; filter execution list by patterns/tags.Please confirm _TestFunction derives tags/useTransactions from code when not provided (non‑cached path), and that tests cover both filtered and unfiltered caching scenarios.
92-97: Incorrect — cache already persists lightweight descriptors.TestRunner._updateFunctionCache (testing/Project/Sources/Classes/TestRunner.4dm, Function _updateFunctionCache) iterates $testFunctions and builds New object(... "name"; $tf.functionName; "tags"; $tf.tags; "useTransactions"; $tf.useTransactions) before saving the cache, so mapping to descriptors in _TestSuite is unnecessary.
Likely an incorrect or invalid review comment.
|
@codex please minimally update the docs with the added user param for forcing a cache refresh. |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
testing/Project/Sources/Classes/TestRunner.4dm (1)
81-88: Deterministic cache signature via sorted keys.Sorting class names before signature fixes nondeterministic hits.
testing/Project/Sources/Classes/_TestSuite.4dm (1)
35-60: Don’t return early unless cached entries actually enqueue tests (and skip cache on forced refresh).Current code returns even when filters reject all cached funcs or cache is stale, yielding empty suites.
- If ($cachedFunctions#Null:C1517) - var $info : Object + // Use cache only if it yields at least one runnable test + If ($cachedFunctions#Null:C1517) && Not:C34(This:C1470.testRunner.forceCacheRefresh) + var $info : Object + var $usedCache : Boolean + $usedCache:=False For each ($info; $cachedFunctions) If (This:C1470._shouldIncludeTestMethod($info.name)) var $func : 4D:C1709.Function $func:=This:C1470.classInstance[$info.name] If ($func#Null:C1517) && (OB Instance of:C1731($func; 4D:C1709.Function)) var $cachedTest : cs:C1710._TestFunction $cachedTest:=cs:C1710._TestFunction.new(This:C1470.class; This:C1470.classInstance; $func; $info.name; ""; $info.tags; $info.useTransactions) If (This:C1470.testRunner#Null:C1517) If (This:C1470.testRunner._shouldIncludeTestByTags($cachedTest)) - This:C1470.testFunctions.push($cachedTest) + This:C1470.testFunctions.push($cachedTest) + $usedCache:=True End if Else - This:C1470.testFunctions.push($cachedTest) + This:C1470.testFunctions.push($cachedTest) + $usedCache:=True End if End if End if End for each - return + If ($usedCache) + return + End if End if
🧹 Nitpick comments (6)
testing/Project/Sources/Classes/TestRunner.4dm (4)
171-188: Sort persisted class names for stable cache; keep write deterministic.Persisting unsorted names can vary order between runs. Sort before writing.
var $names : Collection $names:=[] var $classInfo : 4D:C1709.Class For each ($classInfo; This:C1470._cachedTestClasses) $names.push($classInfo.name) End for each +// Keep on-disk cache stable across runs +$names.sort() var $cacheObj : Object $cacheObj:=New object:C1471("signature"; This:C1470._classStoreSignature; "classes"; $names; "functions"; This:C1470._getFunctionCache()) $cacheFile.setText(JSON Stringify:C1217($cacheObj); "UTF-8")Also applies to: 179-184
147-170: Handle corrupted/old cache files proactively.If JSON Parse fails or signature mismatches, consider deleting/rotating the file to avoid repeated slow reads.
Want a guard that deletes invalid cache files on detection?
189-193: Make refresh flag parsing case‑insensitive and more permissive.Users expect “TRUE/Yes/y/1” to work.
Function _parseCacheOptions() var $params : Object $params:=This:C1470._parseUserParams() - This:C1470.forceCacheRefresh:=($params.refreshCache="1") || ($params.refreshCache="true") + var $rc : Text + $rc:=Lowercase($params.refreshCache || "") + This:C1470.forceCacheRefresh:=($rc="1") || ($rc="true") || ($rc="yes") || ($rc="y")
215-222: Stronger per‑class signature (optional).mtime+size can miss rare edits on FS with coarse timestamps; hashing file contents would be more robust if performance allows.
Need a content‑hash variant (e.g., SHA1 over file text) behind a flag?
testing/Project/Sources/Classes/_TestSuite.4dm (1)
77-81: Unnecessary Null check around $allWrappers push.$allWrappers is initialized to []; drop the guard.
- // Always retain complete discovery set for caching - If ($allWrappers#Null:C1517) - $allWrappers.push($testFunction) - End if + // Always retain complete discovery set for caching + $allWrappers.push($testFunction)testing/Project/Sources/Classes/_TestRunnerTest.4dm (1)
40-61: Add a regression test for “cached but zero enqueued” fallback.Create a scenario where cache exists but no cached entries survive tag/pattern checks or methods were renamed; assert discovery falls back and finds runnable tests.
I can draft this test to simulate stale cache entries and verify the non‑short‑circuit behavior after the fix.
Also applies to: 357-374
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
testing/Project/Sources/Classes/TestRunner.4dm(2 hunks)testing/Project/Sources/Classes/_TestRunnerTest.4dm(6 hunks)testing/Project/Sources/Classes/_TestSuite.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Files:
testing/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/_TestSuite.4dmtesting/Project/Sources/Classes/_TestRunnerTest.4dm
testing/Project/Sources/Classes/*Test.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/*Test.4dm: Name 4D test classes with the suffix "Test" for auto-discovery
Define test methods with names starting with "test_" inside test classes
Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Opt a suite out of parallel execution with the annotation "// #parallel: false"
Automatic transactions are enabled by default; disable per test with "// #transaction: false"
When managing transactions manually, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Files:
testing/Project/Sources/Classes/_TestRunnerTest.4dm
🧠 Learnings (4)
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Name 4D test classes with the suffix "Test" for auto-discovery
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/_TestSuite.4dmtesting/Project/Sources/Classes/_TestRunnerTest.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Define test methods with names starting with "test_" inside test classes
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-09-10T17:16:04.061Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.061Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/_TestSuite.4dm
🔇 Additional comments (8)
testing/Project/Sources/Classes/TestRunner.4dm (2)
75-79: Refresh path correctly resets in‑memory caches.Resetting _cachedTestClasses, _classStoreSignature, and _functionCache on force is correct.
89-97: Honors forceCacheRefresh in load/return paths.Good gating to prevent stale returns when refresh is requested.
testing/Project/Sources/Classes/_TestRunnerTest.4dm (6)
78-78: Updated _filterTestClasses call‑sites look correct.Passing Null for $classNames aligns with new signature.
Also applies to: 268-268, 312-312, 352-352
115-117: Assertion uses wildcard suffix check as intended.Good coverage to ensure only *Test classes are returned.
119-144: Disk‑backed class cache happy‑path covered.Solid verification of create→reload path.
375-401: Function cache persistence test is effective.Exercises suite discovery → cache write → cold read.
402-433: Great: cache retains unfiltered functions across pattern/tag‑filtered runs.These guard against “filter‑shrunk cache” regressions.
Also applies to: 434-467
468-485: Force‑refresh behavior validated.Confirms cache bypass on demand.
| If ($allWrappers#Null:C1517) | ||
| var $sig : Text | ||
| $sig:=This:C1470.testRunner._classFileSignature(This:C1470.class.name) | ||
| // Cache the complete set of discovered test functions so filters don't drop entries | ||
| This:C1470.testRunner._updateFunctionCache(This:C1470.class.name; $sig; $allWrappers) | ||
| End if |
There was a problem hiding this comment.
🧩 Analysis chain
Guard testRunner access and ensure non‑empty cache payload.
Avoid NPE when _TestSuite is used without a runner; also skip no‑op writes.
- If ($allWrappers#Null:C1517)
+ If (This:C1470.testRunner#Null:C1517) && ($allWrappers.length>0)
var $sig : Text
$sig:=This:C1470.testRunner._classFileSignature(This:C1470.class.name)
// Cache the complete set of discovered test functions so filters don't drop entries
This:C1470.testRunner._updateFunctionCache(This:C1470.class.name; $sig; $allWrappers)
End if🏁 Script executed:
#!/usr/bin/env bash
# Verify all _TestSuite.new call sites pass a TestRunner (4th arg)
rg -n -C2 '_TestSuite\.new\(' testing/Project | sed -n '1,160p'Length of output: 13728
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,400p' testing/Project/Sources/Classes/_TestSuite.4dmLength of output: 9330
Guard testRunner access and skip empty-cache writes.
Unprotected calls to This:C1470.testRunner._classFileSignature/_updateFunctionCache will NPE for callers that pass Null:C1517; only update the cache when testRunner != Null and $allWrappers has entries.
Location: testing/Project/Sources/Classes/_TestSuite.4dm — discoverTests() cache-write If block.
- If ($allWrappers#Null:C1517)
+ If (This:C1470.testRunner#Null:C1517) && ($allWrappers.length>0)
var $sig : Text
$sig:=This:C1470.testRunner._classFileSignature(This:C1470.class.name)
// Cache the complete set of discovered test functions so filters don't drop entries
This:C1470.testRunner._updateFunctionCache(This:C1470.class.name; $sig; $allWrappers)
End if📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If ($allWrappers#Null:C1517) | |
| var $sig : Text | |
| $sig:=This:C1470.testRunner._classFileSignature(This:C1470.class.name) | |
| // Cache the complete set of discovered test functions so filters don't drop entries | |
| This:C1470.testRunner._updateFunctionCache(This:C1470.class.name; $sig; $allWrappers) | |
| End if | |
| If (This:C1470.testRunner#Null:C1517) && ($allWrappers.length>0) | |
| var $sig : Text | |
| $sig:=This:C1470.testRunner._classFileSignature(This:C1470.class.name) | |
| // Cache the complete set of discovered test functions so filters don't drop entries | |
| This:C1470.testRunner._updateFunctionCache(This:C1470.class.name; $sig; $allWrappers) | |
| End if |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/_TestSuite.4dm around lines 96–101, the code
calls This:C1470.testRunner._classFileSignature and _updateFunctionCache without
guarding against a null testRunner or an empty $allWrappers, which can cause an
NPE or pointless cache writes; update the If block to first verify
This:C1470.testRunner is not Null and that $allWrappers contains entries before
calling _classFileSignature and _updateFunctionCache, and skip the cache-write
when either condition is false.
gamescoot
left a comment
gamescoot
left a comment
There was a problem hiding this comment.
Looks good. Does this improve time if we use the test cache? Can we ask it to only refresh a single method and leave the rest of the cache as-is?
Summary
Testing
make testhttps://chatgpt.com/codex/tasks/task_e_68c471ba42cc8324a0b9bee7446da040
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation