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

Fixed bug 43993 by removing call to base ViewWillAppear #333

Merged
merged 2 commits into from Oct 4, 2016

Conversation

@adrianknight89
Copy link
Contributor

commented Sep 5, 2016

Description of Change

#274 added call to ViewWillAppear of UITableViewController, but this seems to have broken iOS ListView scrollbar size when the soft keyboard disappears. I removed the base call though I'm not sure why this was necessary. We should either understand the root cause of the problem and invoke base ViewWillAppear or bypass this call as before.

Bugs Fixed

@dnfclas

This comment has been minimized.

Copy link

commented Sep 5, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@samhouts

This comment has been minimized.

Copy link
Member

commented Sep 6, 2016

@adrianknight89: please go ahead and delete the commented code. It was not necessary for #274 and can be removed. Thank you! 👍

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2016

@samhouts Done.

@rmarinho

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

I have doubts that not calling base is the best fix. We should first figure why it breaks with a call to base.

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

@rmarinho My concern is that somebody in the future will think the base call is missing and add it without realizing it creates a nasty bug. @samhouts Should we add back the comments or can you tell us why the base call was not needed?

@samhouts

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

I'm not 100% sure why the base call was originally omitted. I added it while investigating the bug in #274 (it has not previously existed for some time), and then neglected to remove it once I had isolated the cause. @adrianknight89 do you have a test case we can add that will prevent us from regressing?

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

@samhouts I've never written unit/ui tests for XF. I might need to look at Control.ContentOffset and Control.ContentSize properties for this. Is it possible to set up tests to look at these?

@samhouts

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

@adrianknight89

As much as possible, we use Xamarin UITest to create automated regression tests for all bugs. We put the test cases in the Xamarin.Forms.Controls.Issues.Shared project. If you look at the test cases that are there already, you'll see that we use C# for the vast majority of them (easier to write and debug), and we have some helper classes that we derive from.

I haven't tested this yet, but I expect a test for this to look something like the following gist, given the description of the bug on Bugzilla:

https://gist.github.com/samhouts/f3dd0ca2fbd500e0eba339208b0d0d28

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2016

I'm having difficulty running UI tests in XS. This is true for existing tests as well as my own. Simulator hangs while waiting for elements and eventually gives up:

System.Exception : Error while performing WaitForElement(Marked("Success"), "Timed out waiting for element...", null, null, null)
  ----> System.TimeoutException : Timed out waiting for element...

Not sure if I have a setup issue.

app = ConfigureApp.iOS.InstalledApp (AppPaths.BundleId).Debug ()
                .DeviceIdentifier("04167954-C1E8-47CB-B5B4-510E5F5BA5AD")
                .StartApp ();

This is the iPhone 6 simulator I got by running xcrun instruments -s device. It launches successfully but can't finish tests.

I'm looking at the list of bugs in the list view when the gallery app comes up. I don't see B43993 in the list.

@rmarinho

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

@adrianknight89 take a look at the Issues project, copy the _Template page rename it and fill in the blanks, and your code, and in the bottom the UITest.

Try UITest on the device to see if it helps, you will need to add your code signing name .

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2016

Which project(s) do I need to compile once I add a new file?

I created a new file and copied the contents of another test file except that I changed bug ID everywhere. Compiled Xamarin.Forms.Core.iOS.UITests project. I can run the test but it fails with the same timeout exception.

I'm guessing the issue has to do with the [Issue(IssueTracker..)] attribute. Maybe the issue tracker can't find 43993?

Btw, you don't need to wait on me. The above gist looks good (except that I'd add RowSpacing but it wouldn't change anything).

@rmarinho

This comment has been minimized.

Copy link
Member

commented Sep 30, 2016

@adrianknight89 so this is how it works, you add the file and run the gallery.
And you should be able to see the it on the list. The IssueTracker is just some information we add to the tests.
You run the test after you run the gallery and make sure the UITest is there, because the UITest will just open the installed app and try to run it.. When running on the simulator, make sure that where you run the app is the same that UITest is starting.. sometimes it's running a sim with a old version of the app.
Best is to run on device.

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2016

@rmarinho I'm able to deploy the gallery app to the physical device, and I see my entry. All is good, but when I run/debug UI tests, I'm now getting Use CodesignIdentity() to support DeviceAgent with InstalledApp().

I have the correct Signing Identity and Provisioning Profile set on the gallery.

This is my app setup:

app = ConfigureApp.iOS.InstalledApp (AppPaths.BundleId).Debug ()
                //.DeviceIdentifier("04167954-C1E8-47CB-B5B4-510E5F5BA5AD")
                .StartApp ();

I commented the device identifier line so the physical device can be run.

@rmarinho rmarinho merged commit 47b61aa into xamarin:master Oct 4, 2016

rmarinho added a commit that referenced this pull request Oct 4, 2016
Fixed bug 43993 by removing call to base ViewWillAppear (#333)
* Fixed bug 43993 by removing call to base ViewWillAppear

* Removed commented code and reduced nesting
rmarinho added a commit that referenced this pull request Oct 4, 2016
Fixed bug 43993 by removing call to base ViewWillAppear (#333)
* Fixed bug 43993 by removing call to base ViewWillAppear

* Removed commented code and reduced nesting
@rmarinho

This comment has been minimized.

Copy link
Member

commented Oct 4, 2016

@adrianknight89 next build of UITest will fix that, but there's on the release notes of XTC of ios10 that for device in the current version we need to add :

.CodesignIdentity("iPhone Developer: Some One (ABCD1234)")

(replace with your identity)

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2016

@rmarinho When should we expect to see a working test environment? I tried your suggestion above, but now I'm getting another error:

Calabash.XDB.Core.Extensions.DeviceAgentException: Failed to install DeviceAgent

jonathanpeppers pushed a commit to Hitcents/Xamarin.Forms that referenced this pull request Oct 14, 2016
Fixed bug 43993 by removing call to base ViewWillAppear (xamarin#333)
* Fixed bug 43993 by removing call to base ViewWillAppear

* Removed commented code and reduced nesting
jonathanpeppers pushed a commit to Hitcents/Xamarin.Forms that referenced this pull request Oct 14, 2016
jonathanpeppers pushed a commit to Hitcents/Xamarin.Forms that referenced this pull request Oct 28, 2016
@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2016

@samhouts @rmarinho I wrote a UITest for this now that I'm able to run tests on iOS. However, I ran into an issue which I think might be a bug. See https://bugzilla.xamarin.com/show_bug.cgi?id=48926

@samhouts samhouts modified the milestones: 2.3.4, 2.3.3 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.