Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI GUI tests #3

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

CI GUI tests #3

wants to merge 10 commits into from

Conversation

thesiv
Copy link
Owner

@thesiv thesiv commented Oct 22, 2020

Test PR.

@thesiv thesiv force-pushed the ci-gui-tests branch 30 times, most recently from ac4ba27 to 179b6a7 Compare October 25, 2020 17:12
@thesiv thesiv force-pushed the ci-gui-tests branch 9 times, most recently from b3fb6c3 to 9a39a99 Compare October 30, 2020 10:36
Remove `apt-get dist-upgrade` command to not get new untested packages which
could lead to issues.
The main reason to return EXIT_FAILURE if the test was failed is showing
the right status of the test in GitHub CI checks.
If wxUIActionSimulator opens the popup menu using the hot key it will not
exit from wxUIActionSimulator::Char() because it calls wxYield() which will
not returns until the menu hiding. So allow disabling wxYield() to fix it.
wxTimer uses the application traits when constructing but virtual functions
will not work correctly in the constructor (meaning CreateTraits()). So create
the timer in the application initialization function.

wxConfigBase::Get() also creates the config which also uses the application
traits.
The function is required for GTK GUI tests.
Use own UI action simulator in GUI tests which disabling wxYield() to not
stuck inside simulated actions.
Add the delay before showing the license dialog. Without the delay the dialog
will not be shown under GTK3.
Unfortunately dialogs didn't work as expected under GTK3 and the delay is
required inside the modal dialog testing hook invoke handler.
Under GTK up and down arrow keys only change the current radio box item and
space key should be pressed to make the selected item current.
Copy link

@vadz vadz left a comment

Choose a reason for hiding this comment

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

I would love to understand what exactly is the problem with calling wxYield() with GTK 3... It (mostly) works for wx unit tests themselves when executing them in Travis CI environment, so I don't really understand why shouldn't it work in GitHub Actions one. Can you reproduce the problem locally with Xvfb or does it only happen there?

Also, if it affects only wxGTK3, could we enable the tests for wxMSW already? Or are there problems with them too?

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
main_wx_test.cpp Show resolved Hide resolved
Comment on lines -210 to -211
config_ = wxConfigBase::Get();
timer_.Start(100);
Copy link

Choose a reason for hiding this comment

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

This fix seems to be right, but the comment just before Skeleton ctor must be updated to mention that these files can only be initialized in OnInit().

@@ -138,7 +141,7 @@ class Skeleton
wxConfigBase* config_;
DocManagerEx* doc_manager_;
wxDocMDIParentFrame* frame_;
wxTimer timer_;
wxTimerPtr timer_;
Copy link

Choose a reason for hiding this comment

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

AFAICS it should be possible to keep using wxTimer object, just call SetOwner() on it later.

Could you please check if this works? If it does, I'd rather revert the changes here and do it like this.

// Select the last, "Unisex", radio button, by simulating
// down-arrow twice: female --> male, then male --> unisex.
ui.Char(WXK_DOWN);
wxYield();
ui.Char(WXK_DOWN);
wxYield();
ui.Char(WXK_SPACE); // And make it current (required under GTK).
Copy link

Choose a reason for hiding this comment

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

Suggested change
ui.Char(WXK_SPACE); // And make it current (required under GTK).
ui.Char(WXK_SPACE); // And make it current (required under GTK and does no harm under MSW).

Although OTOH it's still a bit wasteful to emulate an unnecessary key press under MSW... maybe we should have some wxUIActionSimulator::SelectRadioButton() helper in the future.

Comment on lines +30 to +31
#include <wx/testing.h>
#include <wx/filedlg.h>
Copy link

Choose a reason for hiding this comment

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

Suggested change
#include <wx/testing.h>
#include <wx/filedlg.h>
#include <wx/filedlg.h>
#include <wx/testing.h>

(should be in alphabetic order, strange that this hasn't been picked up by concinnity check)

@@ -639,6 +671,11 @@ class SkeletonTest final : public Skeleton
{
}

void EnableYield(bool enabled)
{
static_cast<TestEventLoop*>(m_mainLoop)->EnableYield(enabled);
Copy link

Choose a reason for hiding this comment

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

This is pretty dangerous, it would be better if TestEventLoop registered itself as being active with the application object to avoid the potentially bad cast.

Or at least use dynamic_cast<> here.

@thesiv
Copy link
Owner Author

thesiv commented Nov 2, 2020

MSW GUI tests related commits was moved to let-me-illustrate#170.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants