Fix timeout in wxQt tests (Issue #26726)
VZ
vadz created an issue (wxWidgets/wxWidgets#26726)Something has broken wxQt tests, both with Qt 5 and Qt 6. It must be due to one of these commits:
9857cab934 Fix wxHTML line breaks around NBSP-only segments
272da6bcff Fix wxHTML BODY background colour in print rendering
c250dfb706 Fix HTML MIME fallback without registry content type
e04e3260fc Handle context-help mouse capture loss
4e9b924ec6 HTML: Honor TABLE ALIGN as table placement
fbc474f4ed Track active wxGrid editor while editing
cb77a4e904 Correct wxHTML IMG ALIGN=CENTER placement
but I don't know how is this possible knowing that the tests did pass with wxQt for all the PRs these commits were taken from.
@LegalizeAdulthood would you have any idea by chance?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26726)I think the culprit is e04e3260fc.
That commit adds Window::ContextHelpCaptureLost, which starts a wxTimer and then calls wxContextHelp::BeginContextHelp(). This enters wxContextHelp::EventLoop() in src/common/cshelp.cpp, whose loop only dispatches events when wxTheApp->Pending() returns true:
while ( m_inHelp ) { if (wxTheApp->Pending()) wxTheApp->Dispatch(); else wxTheApp->ProcessIdle(); }
But under wxQt, wxQtEventLoopBase::Pending() in src/qt/evtloop.cpp intentionally always returns false; the comment there says the function is effectively useless for wxQt because the old Qt pending-event APIs were deprecated/removed.
So on both Qt 5 and Qt 6 the test's timer never fires, SimulateCaptureLost() is never called, and m_inHelp never becomes false. The result is a timeout.
The other listed commits are mostly deterministic HTML/filesystem/grid tests. This is the only one adding a timer-driven nested GUI loop depending on wxTheApp->Pending(), so it matches the symptom much better.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26726)Do you want to try reverting that one commit?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26726)Thanks, this is what I suspected too, so I'll disable this test with wxQt.
But did I simply fail to notice that the test didn't pass in the PR? I thought it was green...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26726)If you want to revert that one commit, re-open the corresponding bug, I'll get Qt installed locally and test more comprehensively the original fix.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Richard Thomson
LegalizeAdulthood left a comment (wxWidgets/wxWidgets#26726)I checked why this apparently did not fail in the original PR.
The PR and master do not appear to use different wxQt workflow definitions for this: .github/workflows/ci_cmake.yml runs on both pull_request to master and push to master, and the same matrix contains the MSW/MSVC wxQt 5.15 and MSW/MSVC wxQt 6.10 jobs with cmake_tests: ALL and test_gui: 1.
However, PR #26674 did not actually get clean wxQt CMake passes. The CMake workflow for the PR head was cancelled, and both wxQt jobs were cancelled during the Testing step after entering ctest and starting test_gui.exe:
MSW/MSVC wxQt 5.15: job86730502467, run29222571160, cancelled afterStart 2: test_guiMSW/MSVC wxQt 6.10: job86730502450, run29222571160, cancelled afterStart 2: test_gui
In both logs the computed CTest timeout is huge (10000000), so the jobs sat for roughly 6 hours until GitHub cancelled them. The later explicit Testing GUI workflow step was skipped because the earlier Testing step never completed.
So the PR did not prove that the new context-help test passed under wxQt. The relevant wxQt Actions jobs were cancelled/hung, while the legacy combined commit status for the PR head only showed CircleCI/AppVeyor success. One extra wrinkle: the logs show test_gui hanging before printing Window::ContextHelpCaptureLost, so wxQt was already not producing a useful full test_gui pass for that PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#26726)Why not simply change wxContextHelp::EventLoop() implementation to :
diff --git a/src/common/cshelp.cpp b/src/common/cshelp.cpp index f4c730fd63..5fda874bb4 100644 --- a/src/common/cshelp.cpp +++ b/src/common/cshelp.cpp @@ -153,6 +153,7 @@ bool wxContextHelp::EventLoop() while ( m_inHelp ) { +#ifndef __WXQT__
if (wxTheApp->Pending()) { wxTheApp->Dispatch
(); @@ -161,6 +162,9 @@ bool wxContextHelp::EventLoop() { wxTheApp->ProcessIdle(); } +#else + wxTheApp->Dispatch(); +#endif } return true;
Which IMO should fix the problem?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26726)@AliKet If you have the time to test that this fixes the problem, why not. I just wanted to fix this as quickly as possible to allow me to merge other PRs.
Unfortunately this still didn't work out as now it's the Mac workflow which is broken :-(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.