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

Fix failing unit tests in debug mode. #7495

Merged
merged 3 commits into from
Feb 1, 2022

Conversation

grokys
Copy link
Member

@grokys grokys commented Jan 31, 2022

What does the pull request do?

#7369 introduced a validating layer over WindowBase.PlatformImpl that is only enabled in debug mode. This validating layer was causing unit tests to fail in debug mode but not on CI which runs tests in release mode.

Fix the problems:

  • PopupRoot was not correctly disposing itself on Dispose - make it call HandleClosed in order to perform the same steps as when the popup is closed via other means
  • Some unit tests try to access the PlatformImpl to get hold of a mock. Added ValidatingWindowImpl.Unwrap to allow this
  • ValidatingWindowBaseImpl.Activated was setting the wrong property on the wrapped PlatformImpl.

#7369 introduced a validating layer over `WindowBase.PlatformImpl` that is only enabled in debug mode. This validating layer was causing unit tests to fail in debug mode but not on CI which runs tests in release mode.

Fix the problems:

- `PopupRoot` was not correctly disposing itself on `Dispose` - make it call `HandleClosed` in order to perform the same steps as when the popup is closed via other means
- Some unit tests try to access the `PlatformImpl` to get hold of a mock. Added `ValidatingWindowImpl.Unwrap` to allow this
- `ValidatingWindowBaseImpl.Activated` was setting the wrong property on the wrapped `PlatformImpl`.
@grokys grokys requested review from kekekeks and Gillibald January 31, 2022 12:52
@@ -74,7 +74,7 @@ public PopupRoot(TopLevel parent, IPopupImpl impl, IAvaloniaDependencyResolver d
IStyleHost IStyleHost.StylingParent => Parent;

/// <inheritdoc/>
public void Dispose() => PlatformImpl?.Dispose();
public void Dispose() => HandleClosed();
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually dispose the PlatformImpl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, you know what, it doesn't. I saw it was setting it to null but it doesn't first dispose it, i guess because a Closeing the platform impl is the same as disposing it? Looks like we need to dispose the platform impl and the call HandleClosed to set the pointer to null?

It's all a bit confusing...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, Window disposes the PlatformImpl in CloseInternal which is called via HandleClosing, i.e. before HandleClosed, so I've made PopupRoot dispose the PlatformImpl before calling HandleClosed -- does that look right now?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC it was supposed to work like this:

  1. Disposing WindowBase would close the window and will trigger the usual close sequence but without Closing and cancellation
  2. During such procedure the platform impl would call Closed callback

So HandleClosed should be called by platform impl itself

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018357-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018363-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@grokys grokys enabled auto-merge February 1, 2022 07:53
@grokys grokys merged commit 863f019 into master Feb 1, 2022
@grokys grokys deleted the fixes/validating-layer-unit-test-failures branch February 1, 2022 08:12
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0018391-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

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

Successfully merging this pull request may close these issues.

4 participants