Skip to content

Re-enable a test in SmartApplyTests.cs#382

Merged
mkondratek merged 19 commits intomainfrom
mk/fix/smart-apply-test
Oct 28, 2025
Merged

Re-enable a test in SmartApplyTests.cs#382
mkondratek merged 19 commits intomainfrom
mk/fix/smart-apply-test

Conversation

@mkondratek
Copy link
Contributor

@mkondratek mkondratek commented Oct 22, 2025

The SmartApply tests were failing inconsistently due to several issues:

Problems Fixed
Apply button not clickable - The button exists in DOM with tw-hidden class on narrow viewports. Changed wait strategy from WaitForAsync() (waits for visible) to WaitForAsync(State.Attached) (waits for element in DOM), then remove hidden class before clicking.

Missing file context - File context chips weren't appearing in chat input on the first SmartApply test run, causing the LLM to respond without code suggestions (no Apply button). Added a dummy warm-up test to initialize file context functionality. The ApplyLastSuggestionFor helper now waits for context chips before sending prompts.

Insufficient timeout - Increased timeout to 60s to accommodate LLM response times.

Test file improvements - Added intentional typo to Manager.cs to ensure LLM reliably provides code suggestions.

Known Issue

The dummy warm-up test is a workaround. The root cause of why file context chips don't appear on first test run needs investigation - likely a timing or initialization issue.

Test plan

Green and more deterministic CI.

@mkondratek mkondratek self-assigned this Oct 22, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This diff adds extensive debug logging (8 WriteLog statements) to the ApplyLastSuggestionFor test helper method and adds a 60-second wait for the Apply button to appear before interacting with it.

Goal: Improve test reliability and debuggability by adding explicit waits and detailed logging to track test execution flow.

Considerations:

  • The logging density is quite high. While useful for debugging flaky tests, consider whether all 8 log statements need to remain long-term, or if some could be removed once the test is stable.
  • The 60-second timeout on the Apply button wait is generous but may make test failures slower to detect. Ensure this aligns with your test timeout strategy.

View this review on Amp

@github-actions
Copy link

github-actions bot commented Oct 22, 2025

Test Results

27 tests  +1   26 ✅ +2   19m 57s ⏱️ + 9m 33s
 1 suites ±0    1 💤  - 1 
 1 files   ±0    0 ❌ ±0 

Results for commit 93a017b. ± Comparison against base commit d7abb43.

♻️ This comment has been updated with latest results.

@mkondratek mkondratek force-pushed the mk/fix/smart-apply-test branch from 5cba27a to 18b0f75 Compare October 24, 2025 22:20
@mkondratek mkondratek changed the title Enhance logging in ApplyLastSuggestionFor & add a timeout Re-enable a test in SmartApplyTests.cs Oct 24, 2025
public void Print()
{
Console.WriteLine("Hello, World!");
var mesage = "Hello, World!";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is to do something more obviously incorrect here. Before, sometimes Cody suggested no changes (no code snippets). Now we do an explicit typo.

Still not ideal - ideally, we have recordings and the test runs are deterministic.


[VsFact(Version = VsVersion.VS2022)]
public async Task Apply_Suggestion_Is_Modifying_Point_Document()
public async Task Apply_Suggestion_Is_Modifying_Dummy_Document()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn’t expecting this to be the fix, nor was it the outcome I aimed for, but it does solve the issue.

@PiotrKarczmarz PiotrKarczmarz self-requested a review October 28, 2025 13:39
@mkondratek mkondratek merged commit e5f6d67 into main Oct 28, 2025
7 checks passed
@mkondratek mkondratek deleted the mk/fix/smart-apply-test branch October 28, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants