-
Notifications
You must be signed in to change notification settings - Fork 47
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
TestSuite: added "window_modal_bounds_exceeding_work_area" #16
TestSuite: added "window_modal_bounds_exceeding_work_area" #16
Conversation
Regression test for ocornut/imgui#5843
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 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
Test Content
I applied both small tweaks and merged as f4d346d Also:
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 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 |
However mentioning full repro main URL in the PR itself is absolutely fine and recommended. |
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.
Good to know!
Would it make sense to have that built in to Could have a separate
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. |
Almost but.... Typically
Were we to do that, in TestEngine we'd typically use an idiom like |
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. |
Regression test for ocornut/imgui#5843
Verified by intentionally regressing both ocornut/imgui@6e9dfe1 and ocornut/imgui@90e9465 (the loop specifically checks the latter.)