From 600d19e725e71dec5814c4bace1ac39379187b95 Mon Sep 17 00:00:00 2001 From: Patrick Robertson Date: Mon, 23 Mar 2026 21:14:10 +0100 Subject: [PATCH 1/3] Fix up #3103 Regression of a regression + write unit tests to avoid it happening again --- .../QSInterfaceController.m | 23 ++- .../Quicksilver Tests/Quicksilver_Tests.m | 155 ++++++++++++++++++ 2 files changed, 170 insertions(+), 8 deletions(-) diff --git a/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m b/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m index 03ac8fa33..2fd63ef13 100644 --- a/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m +++ b/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m @@ -652,15 +652,22 @@ - (void)executeCommand:(id)sender cont:(BOOL)cont encapsulate:(BOOL)encapsulate NSInteger argumentCount = [action argumentCount]; if (argumentCount == 2) { - // for the case where we require an argument, update the indirects *then* perform the execute - [self updateIndirectObjectsWithCompletion:^{ - [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; - }]; - } else { - // otherwise, just perform the execute straight away - [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; + BOOL indirectIsRequired = ![action indirectOptional]; + BOOL indirectIsInvalid = ![iSelector objectValue]; + BOOL indirectIsTextProxy = [[[iSelector objectValue] primaryType] isEqual:QSTextProxyType]; + if (indirectIsRequired && (indirectIsInvalid || indirectIsTextProxy)) { + // Indirect is needed but not yet available — the async update from + // searchObjectChanged: may not have completed yet. Explicitly fetch + // indirects and execute once they're ready. + [self updateIndirectObjectsWithCompletion:^{ + [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; + }]; + return; + } } - return; + + // Indirect not needed, or user already provided one. Execute immediately. + [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; } - (void)performExecuteAction:(QSAction *)action argumentCount:(NSInteger)argumentCount cont:(BOOL)cont encapsulate:(BOOL)encapsulate { diff --git a/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m b/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m index b1c929ca3..796eff6fb 100644 --- a/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m +++ b/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m @@ -11,7 +11,10 @@ #import "QSController.h" #import "QSObject.h" #import "QSObject_FileHandling.h" +#import "QSObject_StringHandling.h" #import "QSCollectingSearchObjectView.h" +#import "QSAction.h" +#import "QSTextProxy.h" @interface Quicksilver_Tests : XCTestCase @end @@ -242,4 +245,156 @@ - (void)testThirdPaneClosingBehaviour { XCTAssertFalse([self isViewVisible:[i iSelector] forController:i]); } +/** + * Regression test for the race condition introduced by making updateIndirectObjects + * fully async (commit e10140de). When the user presses Return in the 1st pane and + * the action requires an indirect (argumentCount == 2), the async indirect update + * may not have completed yet. executeCommand: should fetch indirects via completion + * block rather than beeping and moving focus to the 3rd pane. + * + * Simulates: type "a" in 1st pane → set a 2-arg action → press Return immediately + */ +- (void)testExecuteCommandWaitsForIndirectsWhenMissing { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Type "a" into dSelector to get a file object + NSEvent *typeAEvent = [NSEvent keyEventWithType:NSEventTypeKeyDown location:NSMakePoint(0, 0) modifierFlags:256 timestamp:15127.081604936 windowNumber:[[i window] windowNumber] context:nil characters:@"a" charactersIgnoringModifiers:@"a" isARepeat:NO keyCode:0]; + [[i dSelector] keyDown:typeAEvent]; + XCTAssertNotNil([[i dSelector] objectValue]); + + // Fire the actions timer to populate actions synchronously + [i fireActionUpdateTimer]; + XCTAssertNotNil([[i aSelector] objectValue]); + + // Set a 2-arg action (Open With). This triggers searchObjectChanged: which + // calls updateIndirectObjects asynchronously (fire-and-forget). + QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; + XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); + [[i aSelector] setObjectValue:openWithAction]; + + // At this point, iSelector is likely empty because the async indirect update + // hasn't completed yet. This is the exact race condition we're testing. + + // Call executeCommand: — this should NOT beep and NOT move focus to iSelector. + // With the fix, it detects the missing indirect and uses the completion path + // to fetch indirects before executing. + [i executeCommand:self]; + + // Wait for the async completion to process + XCTestExpectation *executed = [self expectationWithDescription:@"command executed via completion"]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + dispatch_async(dispatch_get_main_queue(), ^{ + [executed fulfill]; + }); + }); + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + + // The first responder should NOT be iSelector — that would mean the bug occurred + // (beep + focus moved to 3rd pane, requiring a second Return press) + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertTrue(firstResponder != [i iSelector], + @"Focus should not have moved to iSelector — the indirect objects race condition was not handled"); +} + +/** + * Regression test for the bug where executeCommand: unconditionally re-fetched + * indirect objects, overwriting user-entered content in the 3rd pane. + * + * Simulates: select file → choose "Open With" → select an app in 3rd pane → Return + * The user's selection in the 3rd pane must be preserved, not overwritten by a re-fetch. + */ +- (void)testExecuteCommandPreservesUserSelectedIndirect { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Set a file object in dSelector + QSObject *fileObj = [QSObject fileObjectWithPath:@"/System"]; + [[i dSelector] setObjectValue:fileObj]; + XCTAssertNotNil([[i dSelector] objectValue]); + + // Fire the actions timer + [i fireActionUpdateTimer]; + + // Set a 2-arg action + QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; + XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); + [[i aSelector] setObjectValue:openWithAction]; + + // Wait for the indirect objects to be fully populated + XCTestExpectation *indirectsReady = [self expectationWithDescription:@"indirects populated"]; + [i updateIndirectObjectsWithCompletion:^{ + [indirectsReady fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + + // Now simulate the user selecting a specific app in the 3rd pane. + // Use a known file object as the indirect selection. + QSObject *userSelectedApp = [QSObject fileObjectWithPath:@"/Applications/Safari.app"]; + [[i iSelector] setObjectValue:userSelectedApp]; + QSObject *indirectBeforeExecute = [[i iSelector] objectValue]; + XCTAssertNotNil(indirectBeforeExecute, @"iSelector should have the user's selection"); + + // Call executeCommand: — this should use the user's selection, NOT re-fetch + [i executeCommand:self]; + + // Wait for any async operations to settle + XCTestExpectation *settled = [self expectationWithDescription:@"settled"]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + dispatch_async(dispatch_get_main_queue(), ^{ + [settled fulfill]; + }); + }); + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + + // The key assertion: executeCommand: should NOT have moved focus to iSelector + // (which would indicate it re-fetched and found a problem with the indirect) + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertTrue(firstResponder != [i iSelector], + @"executeCommand: should have used the user's existing indirect selection, not re-fetched"); +} + +/** + * End-to-end test for text mode: enter text mode in the 1st pane, + * type text, press Return, and verify the action fires. + * + * Simulates: press "." → type "hello" → press Return + */ +- (void)testTextModeReturnExecutesAction { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Enter text mode on dSelector (equivalent to pressing ".") + [[i dSelector] transmogrify:self]; + NSTextView *textEditor = [[i dSelector] textModeEditor]; + XCTAssertNotNil(textEditor, @"Text mode editor should be active"); + + // Type text into the editor + [textEditor setString:@"hello"]; + // Trigger the text change notification so dSelector updates its objectValue + [[NSNotificationCenter defaultCenter] postNotificationName:NSTextDidChangeNotification object:textEditor]; + + XCTAssertNotNil([[i dSelector] objectValue], @"dSelector should have a text object"); + + // Simulate pressing Return in text mode — this is what textView:doCommandBySelector: + // does: exit text mode, then call executeCommand: + [[i window] makeFirstResponder:[i dSelector]]; + [i executeCommand:self]; + + // Wait for any async completion to process + XCTestExpectation *executed = [self expectationWithDescription:@"text mode command executed"]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + dispatch_async(dispatch_get_main_queue(), ^{ + [executed fulfill]; + }); + }); + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + + // Verify: first responder should NOT be iSelector (would indicate the indirect + // race condition bug — beep + focus to 3rd pane) + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertTrue(firstResponder != [i iSelector], + @"Text mode Return should execute the action, not move focus to the 3rd pane"); +} + @end From 0fce8363f1c17066e10878f0cf03795c7ed083c3 Mon Sep 17 00:00:00 2001 From: Patrick Robertson Date: Mon, 23 Mar 2026 21:36:28 +0100 Subject: [PATCH 2/3] Fix up tests --- .../QSInterfaceControllerTests.m | 124 +++++++++-- .../Quicksilver Tests/Quicksilver_Tests.m | 193 ++---------------- 2 files changed, 130 insertions(+), 187 deletions(-) diff --git a/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m b/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m index 363d96d93..c25b1aac6 100644 --- a/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m +++ b/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m @@ -7,6 +7,7 @@ #import "QSInterfaceController.h" #import "QSController.h" #import "QSObject.h" +#import "QSObject_FileHandling.h" #import "QSSearchObjectView.h" @interface QSInterfaceControllerTests : XCTestCase @@ -14,6 +15,16 @@ @interface QSInterfaceControllerTests : XCTestCase @implementation QSInterfaceControllerTests +- (void)setUp { + [super setUp]; + // Reset action and indirect panes between tests to prevent pollution. + // Do NOT clear dSelector — that destroys its result/source arrays which + // are populated from the catalog and needed for search-based tests. + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + [i clearObjectView:[i aSelector]]; + [i clearObjectView:[i iSelector]]; +} + /// Wait for pending QSGCDAsync + QSGCDMainAsync operations to complete - (void)waitForAsyncUpdates { XCTestExpectation *exp = [self expectationWithDescription:@"async updates"]; @@ -224,19 +235,16 @@ - (void)testTwoArgExecutionDeferredToCompletionBlock { NSInteger originalArgCount = [action argumentCount]; id originalIndirectOptional = [[action actionDict] objectForKey:kActionIndirectOptional]; - // Force the action to appear as a 2-arg action with optional indirect. + // Force the action to appear as a 2-arg action with REQUIRED indirect. // argumentCount == 2 makes executeCommand: take the two-arg code path. - // indirectOptional ensures performExecuteAction: doesn't bail at the - // indirect validity check (since iSelector will be nil). + // Removing indirectOptional (defaults to NO) makes the indirect required, + // which triggers the deferred completion block path when iSelector is nil. [action setArgumentCount:2]; - [[action actionDict] setObject:@YES forKey:kActionIndirectOptional]; + [[action actionDict] removeObjectForKey:kActionIndirectOptional]; // Track whether the notification fires synchronously (before executeCommand: returns) // vs asynchronously (in the completion block, after executeCommand: returns). - // Using queue:nil so the observer fires inline on the posting thread — if the - // notification is posted during executeCommand:, the block runs before it returns. __block BOOL notificationReceived = NO; - XCTestExpectation *exp = [self expectationWithDescription:@"Command eventually executed"]; id observer = [[NSNotificationCenter defaultCenter] addObserverForName:QSCommandExecutedNotification @@ -244,20 +252,20 @@ - (void)testTwoArgExecutionDeferredToCompletionBlock { queue:nil usingBlock:^(NSNotification *notif) { notificationReceived = YES; - [exp fulfill]; }]; [i executeCommand:self]; // Check IMMEDIATELY after executeCommand: returns, before spinning the run loop. - // With the fix: execution is deferred (async via completion block) → NO notification yet. - // Without the fix: execution ran synchronously inside executeCommand: → notification already fired. + // With the fix: indirect is required but missing (iSelector is nil), so execution + // is deferred to the updateIndirectObjectsWithCompletion: callback → no notification yet. + // Without the fix: execution ran synchronously inside executeCommand:. XCTAssertFalse(notificationReceived, - @"For 2-arg actions, execution must be deferred to the completion block. " + @"For 2-arg actions with required indirect, execution must be deferred to the completion block. " @"Synchronous execution indicates the race condition fix is missing."); - // Now wait for the async completion to verify the action DOES eventually fire. - [self waitForExpectationsWithTimeout:5.0 handler:nil]; + // Allow the async completion to drain so it doesn't interfere with other tests + [self waitForAsyncUpdates]; [[NSNotificationCenter defaultCenter] removeObserver:observer]; @@ -270,4 +278,94 @@ - (void)testTwoArgExecutionDeferredToCompletionBlock { } } +/** + * Regression test for the race condition introduced by making updateIndirectObjects + * fully async (commit e10140de). When the user presses Return and the action requires + * an indirect (argumentCount == 2), the async indirect update may not have completed. + * executeCommand: should fetch indirects via completion block rather than beeping. + * + * Uses setObjectValue: directly (not keyDown:) to avoid catalog search dependencies. + */ +- (void)testExecuteCommandWaitsForIndirectsWhenMissing { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Set a file object directly in dSelector + QSObject *fileObj = [QSObject fileObjectWithPath:@"/System"]; + [[i dSelector] setObjectValue:fileObj]; + XCTAssertNotNil([[i dSelector] objectValue]); + + // Fire the actions timer to populate actions synchronously + [i fireActionUpdateTimer]; + XCTAssertNotNil([[i aSelector] objectValue]); + + // Set a 2-arg action (Open With). This triggers searchObjectChanged: which + // calls updateIndirectObjects asynchronously (fire-and-forget). + QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; + XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); + [[i aSelector] setObjectValue:openWithAction]; + + // At this point, iSelector is likely empty because the async indirect update + // hasn't completed yet. This is the exact race condition we're testing. + + // Call executeCommand: — this should NOT beep and NOT move focus to iSelector. + [i executeCommand:self]; + + // Wait for the async completion to process + [self waitForAsyncUpdates]; + + // The first responder should NOT be iSelector — that would mean the bug occurred + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertNotEqual(firstResponder, [i iSelector], + @"Focus should not have moved to iSelector — the indirect objects race condition was not handled"); +} + +/** + * Regression test: executeCommand: must NOT unconditionally re-fetch indirect objects, + * as that would overwrite user-entered content in the 3rd pane. + * + * Simulates: select file → choose "Open With" → select an app in 3rd pane → Return + * The user's selection in the 3rd pane must be preserved, not overwritten. + */ +- (void)testExecuteCommandPreservesUserSelectedIndirect { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Set a file object in dSelector + QSObject *fileObj = [QSObject fileObjectWithPath:@"/System"]; + [[i dSelector] setObjectValue:fileObj]; + XCTAssertNotNil([[i dSelector] objectValue]); + + // Fire the actions timer + [i fireActionUpdateTimer]; + + // Set a 2-arg action + QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; + XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); + [[i aSelector] setObjectValue:openWithAction]; + + // Wait for the indirect objects to be fully populated + XCTestExpectation *indirectsReady = [self expectationWithDescription:@"indirects populated"]; + [i updateIndirectObjectsWithCompletion:^{ + [indirectsReady fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + + // Now simulate the user selecting a specific app in the 3rd pane + QSObject *userSelectedApp = [QSObject fileObjectWithPath:@"/Applications/Safari.app"]; + [[i iSelector] setObjectValue:userSelectedApp]; + XCTAssertNotNil([[i iSelector] objectValue], @"iSelector should have the user's selection"); + + // Call executeCommand: — this should use the user's selection, NOT re-fetch + [i executeCommand:self]; + + // Wait for any async operations to settle + [self waitForAsyncUpdates]; + + // executeCommand: should NOT have moved focus to iSelector + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertNotEqual(firstResponder, [i iSelector], + @"executeCommand: should have used the user's existing indirect selection, not re-fetched"); +} + @end diff --git a/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m b/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m index 796eff6fb..12d06127f 100644 --- a/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m +++ b/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m @@ -11,10 +11,7 @@ #import "QSController.h" #import "QSObject.h" #import "QSObject_FileHandling.h" -#import "QSObject_StringHandling.h" #import "QSCollectingSearchObjectView.h" -#import "QSAction.h" -#import "QSTextProxy.h" @interface Quicksilver_Tests : XCTestCase @end @@ -42,7 +39,7 @@ - (void)testRightArrowIntoSynonym { [proxy setIdentifier:[NSString stringWithFormat:@"QSUserDefinedProxy:%@", name]]; [proxy setName:name]; [proxy setObject:targetID forMeta:@"target"]; - + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; [[i dSelector] selectObjectValue:proxy]; [[i dSelector] moveRight:self]; @@ -52,23 +49,23 @@ - (void)testRightArrowIntoSynonym { - (void)testCreateObjectFromRTFClipboard { // create an rtf string in the clipboard NSPasteboard *pboard = [NSPasteboard generalPasteboard]; - + NSAttributedString *rtfString = [[NSAttributedString alloc] initWithRTF:[@"{\\rtf1\\ansi\\ansicpg1252\\cocoartf1187\\cocoasubrtf340\n{\\fonttbl\\f0\\fnil\\fcharset0 Monaco;}\n{\\colortbl;\\red255\\green0\\blue0;}\n\\margl1440\\margr1440\\vieww10800\\viewh8400\\viewkind0\n\\pard\\tx720\\tx1440\\tx2160\\tx2880\\tx3600\\tx4320\\tx5040\\tx5760\\tx6480\\tx7200\\tx7920\\tx8640\\ql\\qnatural\\pardirnatural\n\n\\f0\\fs24 \\cf1 hello}" dataUsingEncoding:NSUTF8StringEncoding] documentAttributes:nil]; [pboard clearContents]; [pboard writeObjects:@[rtfString]]; - + // create a QSObject from the clipboard QSObject *obj = [QSObject objectWithPasteboard:pboard]; XCTAssertTrue([obj containsType:NSPasteboardTypeRTF] && [[obj primaryType] isEqualToString:NSPasteboardTypeRTF]); - + // now write this object to the pasteboard [pboard clearContents]; XCTAssertTrue([obj putOnPasteboard:pboard]); - + // read the object back from the pasteboard QSObject *obj2 = [QSObject objectWithPasteboard:pboard]; XCTAssertTrue([obj2 containsType:NSPasteboardTypeRTF] && [[obj2 primaryType] isEqualToString:NSPasteboardTypeRTF]); - + } // test to make sure when file objects are added to the clipboard, a string of their path is also copied @@ -79,12 +76,12 @@ - (void)testAddingFileObjectToPasteboard { XCTAssertEqualObjects([obj singleFilePath], path); XCTAssertTrue([obj putOnPasteboard:pboard] == YES); XCTAssertTrue([[pboard types] containsObject:NSPasteboardTypeFileURL]); - + // for file objects, we don't write the QSTextType to the pasteboard - that messes with drag/drop in some applications XCTAssertFalse([[pboard types] containsObject:QSTextType]); NSArray *a = [pboard propertyListForType:NSPasteboardTypeFileURL]; XCTAssertEqualObjects(a, @"file:///usr/bin/cd"); - + // try this for an imagined type that already has a string type set: obj = [QSObject fileObjectWithPath:path]; NSString *textString = @"My Important String"; @@ -112,13 +109,13 @@ - (void)testAddingFileObjectToPasteboardWithMultiplePaths { return [pbitem propertyListForType:NSPasteboardTypeFileURL]; }]; XCTAssertEqualObjects(a, fileArray); - + // now try re-creating the object from the pasteboard QSObject *newObj = [QSObject objectWithPasteboard:pboard]; XCTAssertTrue([newObj containsType:QSFilePathType]); XCTAssertEqualObjects([newObj arrayForType:QSFilePathType], filePathArray); } - + /** * In order to run these tests, you must select 'Quicksilver' as the scheme, as opposed to 'Quicksilver Distribution' @@ -129,7 +126,7 @@ - (void)testAddingFileObjectToPasteboardWithMultiplePaths { */ - (void)testClearingFirstPane { - + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; // Test is sure to fail if we can't get the interface controller @@ -137,7 +134,7 @@ - (void)testClearingFirstPane // Assumes the current interface can collect QSCollectingSearchObjectView *dSelector = (QSCollectingSearchObjectView *)[i dSelector]; - + // Make a couple of file objects and add them to the dSelector NSArray *paths = @[@"/System", @"/Users"]; NSArray *objs = @[[QSObject fileObjectWithPath:paths[0]], [QSObject fileObjectWithPath:paths[1]]]; @@ -145,15 +142,15 @@ - (void)testClearingFirstPane [dSelector setObjectValue:o]; [dSelector collect:o]; } - + // Ensure the collected objects are the same as those added XCTAssertEqualObjects([[dSelector objectValue] splitObjects], objs, @"Collected objects aren't correct"); - + // Call some action on the objects (Get File URL known to be problematic QSAction *getFileURLAction = [QSAction actionWithIdentifier:@"FileGetURLAction"]; [[i aSelector] setObjectValue:getFileURLAction]; [i executeCommandThreaded]; - + // Ensure the returned files are what we expected (for this action) NSString *fileURLs = [[NSURL performSelector:@selector(fileURLWithPath:) onObjectsInArray:paths returnValues:YES] componentsJoinedByString:@"\n"]; QSObject *fileURLStringObject = [QSObject objectWithType:QSTextType value:fileURLs name:fileURLs]; @@ -169,16 +166,16 @@ - (void)testClearingSearchStringOnTrigger { NSEvent *typeAEvent = [NSEvent keyEventWithType:NSEventTypeKeyDown location:NSMakePoint(0, 0) modifierFlags:256 timestamp:15127.081604936 windowNumber:[[i window] windowNumber] context:nil characters:@"a" charactersIgnoringModifiers:@"a" isARepeat:NO keyCode:0]; // Simulate typing 'a' into the dSelector [[i dSelector] keyDown:typeAEvent]; - + XCTAssertEqualObjects(@"a", [[i dSelector] searchString], @"The search string typed into the dSelector is incorrect"); - + // Simulate opening a trigger like iTunes' 'Search Artists' (a.k.a. a search children action) QSAction *searchChildrenAction = [QSAction actionWithIdentifier:@"QSObjectSearchChildrenAction"]; [[i aSelector] setObjectValue:searchChildrenAction]; [i executeCommandThreaded]; - + [[i dSelector] keyDown:typeAEvent]; - + XCTAssertEqualObjects(@"a", [[i dSelector] searchString], @"The previous search string was not cleared when invoking a 'search children' command"); } @@ -245,156 +242,4 @@ - (void)testThirdPaneClosingBehaviour { XCTAssertFalse([self isViewVisible:[i iSelector] forController:i]); } -/** - * Regression test for the race condition introduced by making updateIndirectObjects - * fully async (commit e10140de). When the user presses Return in the 1st pane and - * the action requires an indirect (argumentCount == 2), the async indirect update - * may not have completed yet. executeCommand: should fetch indirects via completion - * block rather than beeping and moving focus to the 3rd pane. - * - * Simulates: type "a" in 1st pane → set a 2-arg action → press Return immediately - */ -- (void)testExecuteCommandWaitsForIndirectsWhenMissing { - QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; - XCTAssertNotNil(i); - - // Type "a" into dSelector to get a file object - NSEvent *typeAEvent = [NSEvent keyEventWithType:NSEventTypeKeyDown location:NSMakePoint(0, 0) modifierFlags:256 timestamp:15127.081604936 windowNumber:[[i window] windowNumber] context:nil characters:@"a" charactersIgnoringModifiers:@"a" isARepeat:NO keyCode:0]; - [[i dSelector] keyDown:typeAEvent]; - XCTAssertNotNil([[i dSelector] objectValue]); - - // Fire the actions timer to populate actions synchronously - [i fireActionUpdateTimer]; - XCTAssertNotNil([[i aSelector] objectValue]); - - // Set a 2-arg action (Open With). This triggers searchObjectChanged: which - // calls updateIndirectObjects asynchronously (fire-and-forget). - QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; - XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); - [[i aSelector] setObjectValue:openWithAction]; - - // At this point, iSelector is likely empty because the async indirect update - // hasn't completed yet. This is the exact race condition we're testing. - - // Call executeCommand: — this should NOT beep and NOT move focus to iSelector. - // With the fix, it detects the missing indirect and uses the completion path - // to fetch indirects before executing. - [i executeCommand:self]; - - // Wait for the async completion to process - XCTestExpectation *executed = [self expectationWithDescription:@"command executed via completion"]; - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - dispatch_async(dispatch_get_main_queue(), ^{ - [executed fulfill]; - }); - }); - [self waitForExpectationsWithTimeout:5.0 handler:nil]; - - // The first responder should NOT be iSelector — that would mean the bug occurred - // (beep + focus moved to 3rd pane, requiring a second Return press) - NSResponder *firstResponder = [[i window] firstResponder]; - XCTAssertTrue(firstResponder != [i iSelector], - @"Focus should not have moved to iSelector — the indirect objects race condition was not handled"); -} - -/** - * Regression test for the bug where executeCommand: unconditionally re-fetched - * indirect objects, overwriting user-entered content in the 3rd pane. - * - * Simulates: select file → choose "Open With" → select an app in 3rd pane → Return - * The user's selection in the 3rd pane must be preserved, not overwritten by a re-fetch. - */ -- (void)testExecuteCommandPreservesUserSelectedIndirect { - QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; - XCTAssertNotNil(i); - - // Set a file object in dSelector - QSObject *fileObj = [QSObject fileObjectWithPath:@"/System"]; - [[i dSelector] setObjectValue:fileObj]; - XCTAssertNotNil([[i dSelector] objectValue]); - - // Fire the actions timer - [i fireActionUpdateTimer]; - - // Set a 2-arg action - QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; - XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); - [[i aSelector] setObjectValue:openWithAction]; - - // Wait for the indirect objects to be fully populated - XCTestExpectation *indirectsReady = [self expectationWithDescription:@"indirects populated"]; - [i updateIndirectObjectsWithCompletion:^{ - [indirectsReady fulfill]; - }]; - [self waitForExpectationsWithTimeout:5.0 handler:nil]; - - // Now simulate the user selecting a specific app in the 3rd pane. - // Use a known file object as the indirect selection. - QSObject *userSelectedApp = [QSObject fileObjectWithPath:@"/Applications/Safari.app"]; - [[i iSelector] setObjectValue:userSelectedApp]; - QSObject *indirectBeforeExecute = [[i iSelector] objectValue]; - XCTAssertNotNil(indirectBeforeExecute, @"iSelector should have the user's selection"); - - // Call executeCommand: — this should use the user's selection, NOT re-fetch - [i executeCommand:self]; - - // Wait for any async operations to settle - XCTestExpectation *settled = [self expectationWithDescription:@"settled"]; - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - dispatch_async(dispatch_get_main_queue(), ^{ - [settled fulfill]; - }); - }); - [self waitForExpectationsWithTimeout:5.0 handler:nil]; - - // The key assertion: executeCommand: should NOT have moved focus to iSelector - // (which would indicate it re-fetched and found a problem with the indirect) - NSResponder *firstResponder = [[i window] firstResponder]; - XCTAssertTrue(firstResponder != [i iSelector], - @"executeCommand: should have used the user's existing indirect selection, not re-fetched"); -} - -/** - * End-to-end test for text mode: enter text mode in the 1st pane, - * type text, press Return, and verify the action fires. - * - * Simulates: press "." → type "hello" → press Return - */ -- (void)testTextModeReturnExecutesAction { - QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; - XCTAssertNotNil(i); - - // Enter text mode on dSelector (equivalent to pressing ".") - [[i dSelector] transmogrify:self]; - NSTextView *textEditor = [[i dSelector] textModeEditor]; - XCTAssertNotNil(textEditor, @"Text mode editor should be active"); - - // Type text into the editor - [textEditor setString:@"hello"]; - // Trigger the text change notification so dSelector updates its objectValue - [[NSNotificationCenter defaultCenter] postNotificationName:NSTextDidChangeNotification object:textEditor]; - - XCTAssertNotNil([[i dSelector] objectValue], @"dSelector should have a text object"); - - // Simulate pressing Return in text mode — this is what textView:doCommandBySelector: - // does: exit text mode, then call executeCommand: - [[i window] makeFirstResponder:[i dSelector]]; - [i executeCommand:self]; - - // Wait for any async completion to process - XCTestExpectation *executed = [self expectationWithDescription:@"text mode command executed"]; - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - dispatch_async(dispatch_get_main_queue(), ^{ - [executed fulfill]; - }); - }); - [self waitForExpectationsWithTimeout:5.0 handler:nil]; - - // Verify: first responder should NOT be iSelector (would indicate the indirect - // race condition bug — beep + focus to 3rd pane) - NSResponder *firstResponder = [[i window] firstResponder]; - XCTAssertTrue(firstResponder != [i iSelector], - @"Text mode Return should execute the action, not move focus to the 3rd pane"); -} - @end From c2de0f16f334aead275a797cc91be292bef264d8 Mon Sep 17 00:00:00 2001 From: Patrick Robertson Date: Tue, 24 Mar 2026 09:16:08 +0100 Subject: [PATCH 3/3] Finally (hopefully) fix flaky tests --- .../QSInterfaceControllerTests.m | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m b/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m index c25b1aac6..bf46ed2c6 100644 --- a/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m +++ b/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m @@ -295,8 +295,9 @@ - (void)testExecuteCommandWaitsForIndirectsWhenMissing { [[i dSelector] setObjectValue:fileObj]; XCTAssertNotNil([[i dSelector] objectValue]); - // Fire the actions timer to populate actions synchronously - [i fireActionUpdateTimer]; + // Populate actions directly (don't rely on timer — it may have been + // invalidated by a previous test calling executeCommand:) + [i updateActionsNow]; XCTAssertNotNil([[i aSelector] objectValue]); // Set a 2-arg action (Open With). This triggers searchObjectChanged: which @@ -308,11 +309,26 @@ - (void)testExecuteCommandWaitsForIndirectsWhenMissing { // At this point, iSelector is likely empty because the async indirect update // hasn't completed yet. This is the exact race condition we're testing. - // Call executeCommand: — this should NOT beep and NOT move focus to iSelector. + // Listen for QSCommandExecutedNotification — posted after the action runs. + // With the fix: executeCommand: detects the missing indirect, defers to the + // completion block of updateIndirectObjectsWithCompletion:, which fetches + // indirects on a background thread then executes on main. The notification + // fires once execution completes, giving us a deterministic wait point. + // Without the fix: executeCommand: would synchronously beep and focus iSelector. + XCTestExpectation *commandExecuted = [self expectationWithDescription:@"command executed via deferred completion"]; + id observer = [[NSNotificationCenter defaultCenter] + addObserverForName:QSCommandExecutedNotification + object:nil + queue:nil + usingBlock:^(NSNotification *notif) { + [commandExecuted fulfill]; + }]; + [i executeCommand:self]; - // Wait for the async completion to process - [self waitForAsyncUpdates]; + // Wait for the command to actually execute (deterministic — no GCD timing hacks) + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + [[NSNotificationCenter defaultCenter] removeObserver:observer]; // The first responder should NOT be iSelector — that would mean the bug occurred NSResponder *firstResponder = [[i window] firstResponder]; @@ -336,8 +352,8 @@ - (void)testExecuteCommandPreservesUserSelectedIndirect { [[i dSelector] setObjectValue:fileObj]; XCTAssertNotNil([[i dSelector] objectValue]); - // Fire the actions timer - [i fireActionUpdateTimer]; + // Populate actions directly (don't rely on timer state) + [i updateActionsNow]; // Set a 2-arg action QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"];