TestSuite: added "window_modal_bounds_exceeding_work_area"

[PathogenDavid]

Regression test for https://github.com/ocornut/imgui/issues/5843

Verified by intentionally regressing both https://github.com/ocornut/imgui/commit/6e9dfe1de1131635128e01cc6ae2420c781d88d1 and https://github.com/ocornut/imgui/commit/90e9465fa579efcac20f250433ff6307980d9bbc (the loop specifically checks the latter.)

[ocornut]

Thanks David!

I'll be over-zealous with my comments here for the sake of sharing some ideas (may be useful to others or later for documentation).

About version checks.

We like to use #if IMGUI_VERSION_NUM > XXXXX checks in tests but it's not a strict necessity. We started doing that along with making the integer version increase more frequent in main library (which is always useful to narrow down version in repros). (A) From the point of view of the test suite it helps narrow down the origin of a change, doubled with the mention of issue #, (B) but it was also somehow designed to reduce pressure of merging master into docking too frequently: often a new fix will pass in master but not in docking. I merge docking frequently but it would be cumbersome to do it for every commit.

In this case only (A) apply since the fix was applied a while ago already, and it is of less importance since there's an associated issue # in the comment. For completeness I would add a version check if it felt it wasn't a hassle to find it. Picking a nearby version is ok even if it means in theory a few of the previous commits wouldn't pass the test. We don't expect Test Suite to successfully run with any older version anyway.

The situation is different for Test Engine itself where we more actively try to make changes work with a wider number of main library version, to make it easier for people to update either side only when possible.

Coding Style

{ 1.f, ImGui::GetIO().DisplaySize.y * 1.5f } I think I'm ready to accept this initializer style in Test Suite but note I'm not sure we can use it in Test Engine which is a reused library (maybe we can, have to do some researchs).

Test Content

ImGui::Begin("Test"); we try to use ImGui::Begin("Test Window", NULL, ImGuiWindowFlags_NoSavedSettings); most of the time for consistency.

io.ConfigWindowsMoveFromTitleBarOnly = true; no issue. In avoidance of doubt: the content of IO is backed up and restored at the end of the test, so if the test were to fail mid-way, this wouldn't be damaging and is ok.

I applied both small tweaks and merged as f4d346d

Also:

ImGuiWindow* window = ctx->GetWindowByRef("Modal");
[...]
IM_CHECK(work_rect.Contains(window->Rect()));

I noticed that grabbing a window pointer without checking it has been an occasional reason for crashes in the test suite, e.g. crashes occasionally appears when some condition or behavior changes. Nowadays I try to add a IM_CHECK(window != NULL); test for those. It's not important as when it actually happens the first thing we'd do would be to add that IM_CHECK anyway, then investigate the test failing.

Commit

As a small niggle, we realized that mentioning main repo issue by URL in the "draft" commits became noisy as every force-push or rebase would spam original issue. I think I took the habit to write 1234 in commit description instead of #1234 (would tag wrong repo) or github.com/..../1234 (would spam), and occasionally on final push we can change to an URL and if we forget it's not the end of the world.

[ocornut]

As a small niggle, we realized that mentioning main repo issue by URL in the "draft" commits became noisy as every force-push or rebase would spam original issue. I think I took the habit to write 1234 in commit description instead of #1234 (would tag wrong repo) or github.com/..../1234 (would spam), and occasionally on final push we can change to an URL and if we forget it's not the end of the world.

However mentioning full repro main URL in the PR itself is absolutely fine and recommended. Hopefully we add test-engine-repo # in merged commits so the link chain works naturally without even adding main-repo # in commit message.

[PathogenDavid]

reduce pressure of merging master into docking too frequently

This is the context I was missing! You merge them so often I forgot they do actually drift occasionally. I'll add version checks to future tests.

the content of IO is backed up and restored at the end of the test

Good to know!

Nowadays I try to add a IM_CHECK(window != NULL); test for those.

Would it make sense to have that built in to GetWindowByRef to rule out this mistake entirely?

Could have a separate TryGetWindowByRef for cases where you might expect the window to not exist.

As a small niggle, we realized that mentioning main repo issue by URL in the "draft" commits became noisy as every force-push or rebase would spam original issue.

Yes I am quite annoyed by that too. I wish GitHub would avoid mentioning commits from forks and/or orphaned commits in those logs. I'll avoid the URL for future commits and just put it in the PR.

[ocornut]

Would it make sense to have that built in to GetWindowByRef to rule out this mistake entirely?

Almost but....GetWindowByRef() could set the test status to "Error" but that doesn't stop the test function from running and if the following code tries to use this pointer it would crash the same way.

Typically IM_CHECK() does a return (also see IM_CHECK_RETV() and IM_CHECK_NO_RET()). And all ctx-> function are doing an early out when the test status is errored, but they are case where some code still runs for a while.

Could have a separate TryGetWindowByRef for cases where you might expect the window to not exist.

Were we to do that, in TestEngine we'd typically use an idiom like GetWindowByRef(..., ImGuiTestOpFlags_NoError);

[PathogenDavid]

Typically IM_CHECK() does a return (also see IM_CHECK_RETV() and IM_CHECK_NO_RET()). And all ctx-> function are doing an early out when the test status is errored, but they are case where some code still runs for a while.

Ah, in my head I was imagining it was either throwing an exception to abort the coroutine early or self-exiting the test thread. I haven't had much time to poke around in the internals yet.