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

[XCUITests] Launch tests for all controls #1461

Merged
merged 8 commits into from
Dec 22, 2022

Conversation

joannaquu
Copy link
Contributor

@joannaquu joannaquu commented Dec 19, 2022

Platforms Impacted

  • iOS
  • macOS

Description of changes

Binary change:

File Before After Delta
libFluentUI.a 54,943,520 bytes 54,943,520 bytes 0 bytes

Added a launch test for every control that simply opens each control and ensures the demo app does not crash. setUpWithError will launch the app, and then navigate to the correct page (UIKit and SwiftUI). In the case that the demo app starts on the SideTarBar page, it will use the dismiss button instead of the back button.

Iterations this PR has undergone

  • Originally, these tests were written in Swift and each control contained the same code/logic to navigate to their corresponding page.
  • An attempt was made to create a base class in Swift to simplify the code and remove the duplicated code. However, customizing each test to each control required that we pass controlName to the base class. This would call init(invocation:), which was an issue because NSInvocation is unimplemented on Swift. See link. Thus, it was rewritten to use objective c.
  • Since most of the controls in the FUA repo, as well as other XCUITests in the monorepo use Swift, the tests were changed back to Swift, now using an extension on XCTestCase to address the code duplication.
  • Another attempt was made to use a base class in Swift. This time, to avoid running into the init(invocation:) error, the controlName was stored as a property that each test page would override.

This PR does not contain any tests that are specific to each control; specific tests will be added in subsequent PRs.

Resources others may find useful:

Verification

UIKit SwiftUI
Simulator Screen Recording - Clone 3 of iPhone 14 Pro - 2022-12-19 at 13 54 55 Simulator Screen Recording - Clone 3 of iPhone 14 Pro - 2022-12-19 at 13 54 14

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)
Microsoft Reviewers: Open in CodeFlow

@joannaquu joannaquu requested a review from a team as a code owner December 19, 2022 22:57
@lyzhan7
Copy link
Contributor

lyzhan7 commented Dec 21, 2022

Some thoughts -

  • So it seems like if we want to use a base class, we'll have to write all our tests in objective c instead of swift? I wonder if that trade off is worth it, maybe we can talk offline.
  • Also thinking that setting up one or two tests to start with and getting those nailed down first would save some time in the future, instead of changing all the tests at once

@edjamesmsft
Copy link
Contributor

  • So it seems like if we want to use a base class, we'll have to write all our tests in objective c instead of swift? I wonder if that trade off is worth it, maybe we can talk offline.

One benefit of writing the tests in objc is that we have low objc coverage currently, and that's where our big breaking changes end up happening. So I think writing these in objc isn't necessarily a total downside!

@lyzhan7
Copy link
Contributor

lyzhan7 commented Dec 21, 2022

  • So it seems like if we want to use a base class, we'll have to write all our tests in objective c instead of swift? I wonder if that trade off is worth it, maybe we can talk offline.

One benefit of writing the tests in objc is that we have low objc coverage currently, and that's where our big breaking changes end up happening. So I think writing these in objc isn't necessarily a total downside!

Hmm interesting, how does writing the tests in objc instead of swift increase our code coverage of objc? If we're still trying to write the same kind of tests (ex. does clicking this button fire the right event), that seems like it would be testing the same code. Do you mean that if we're setting up the tests in obj c we'll be able to test some code that we wouldn't be able to if the tests were set up in swift?

@edjamesmsft
Copy link
Contributor

  • So it seems like if we want to use a base class, we'll have to write all our tests in objective c instead of swift? I wonder if that trade off is worth it, maybe we can talk offline.

One benefit of writing the tests in objc is that we have low objc coverage currently, and that's where our big breaking changes end up happening. So I think writing these in objc isn't necessarily a total downside!

Hmm interesting, how does writing the tests in objc instead of swift increase our code coverage of objc? If we're still trying to write the same kind of tests (ex. does clicking this button fire the right event), that seems like it would be testing the same code. Do you mean that if we're setting up the tests in obj c we'll be able to test some code that we wouldn't be able to if the tests were set up in swift?

If we actually do any serious exercising of calling APIs for any given component in objc, we'll have coverage of those calling patterns in objc. Swift and objc do not have the same rules for things like default values for inits, for example, and we have absolutely introduced breaking changes in objc because "it worked in swift fine!", but not so for objc. It's not absolutely critical, but I do believe we should be using our library the way our customers will, and that means we need SOME coverage in objc. If this is one way to get that, it's not a bad thing.

@joannaquu
Copy link
Contributor Author

  • So it seems like if we want to use a base class, we'll have to write all our tests in objective c instead of swift? I wonder if that trade off is worth it, maybe we can talk offline.

One benefit of writing the tests in objc is that we have low objc coverage currently, and that's where our big breaking changes end up happening. So I think writing these in objc isn't necessarily a total downside!

Hmm interesting, how does writing the tests in objc instead of swift increase our code coverage of objc? If we're still trying to write the same kind of tests (ex. does clicking this button fire the right event), that seems like it would be testing the same code. Do you mean that if we're setting up the tests in obj c we'll be able to test some code that we wouldn't be able to if the tests were set up in swift?

If we actually do any serious exercising of calling APIs for any given component in objc, we'll have coverage of those calling patterns in objc. Swift and objc do not have the same rules for things like default values for inits, for example, and we have absolutely introduced breaking changes in objc because "it worked in swift fine!", but not so for objc. It's not absolutely critical, but I do believe we should be using our library the way our customers will, and that means we need SOME coverage in objc. If this is one way to get that, it's not a bad thing.

In that case, would writing these tests in objc be preferred? I've been working on changing this back to Swift so any preferences?

@lyzhan7
Copy link
Contributor

lyzhan7 commented Dec 21, 2022

  • So it seems like if we want to use a base class, we'll have to write all our tests in objective c instead of swift? I wonder if that trade off is worth it, maybe we can talk offline.

One benefit of writing the tests in objc is that we have low objc coverage currently, and that's where our big breaking changes end up happening. So I think writing these in objc isn't necessarily a total downside!

Hmm interesting, how does writing the tests in objc instead of swift increase our code coverage of objc? If we're still trying to write the same kind of tests (ex. does clicking this button fire the right event), that seems like it would be testing the same code. Do you mean that if we're setting up the tests in obj c we'll be able to test some code that we wouldn't be able to if the tests were set up in swift?

If we actually do any serious exercising of calling APIs for any given component in objc, we'll have coverage of those calling patterns in objc. Swift and objc do not have the same rules for things like default values for inits, for example, and we have absolutely introduced breaking changes in objc because "it worked in swift fine!", but not so for objc. It's not absolutely critical, but I do believe we should be using our library the way our customers will, and that means we need SOME coverage in objc. If this is one way to get that, it's not a bad thing.

Ah I see, so FUA component APIs can be called by either swift or objective c code, and writing the tests in objective c will not only test the component itself but also will test whether calling the APIs with objective c will work. I wonder if just writing objective c tests will cover everything, or if we would actually want tests in both objective c as well as swift? I.e. maybe there is also some behavior that can be only tested in swift

Copy link
Contributor

@lyzhan7 lyzhan7 left a comment

Choose a reason for hiding this comment

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

Left a couple comments you could consider (although take a look at the one where one of the asserts seems different from the other ones), overall looks good! Thanks also for filling out the PR description - might also want to update it again now that we got the base test class working

@lyzhan7
Copy link
Contributor

lyzhan7 commented Dec 22, 2022

@huwilkes @edjamesmsft do either of you know whether the changes to the xcscheme files should be there? A modification to the development one makes sense to me but I wonder if it makes sense that the release/dogfood ones were changed

@huwilkes
Copy link
Collaborator

@huwilkes @edjamesmsft do either of you know whether the changes to the xcscheme files should be there? A modification to the development one makes sense to me but I wonder if it makes sense that the release/dogfood ones were changed

I think if we want to be able run the tests on any scheme, they would all need the change. If we only wanted to be able to run tests on debug we could revert the others, but since it's just in the demo app it's probably okay to leave the change in all three.

@joannaquu joannaquu merged commit 95e69eb into microsoft:main Dec 22, 2022
@joannaquu joannaquu deleted the joannaqu/launch-tests branch December 27, 2022 16:57
@edjamesmsft edjamesmsft mentioned this pull request Jan 25, 2023
11 tasks
edjamesmsft added a commit that referenced this pull request Jan 25, 2023
* SegmentedControl code clean up (#1432)

* fix rotation bug on segmented control (#1411)

* Fix pillMaskedLabelContainerView comment

* Fix update logic

* Make tokenSink and animation duration private

* Update selectionView on token update

Co-authored-by: Harrie Shin <hyshin@microsoft.com>

* Update to checkout v3 (#1459)

* Adding additional leading view to TableViewCell (#1446)

* Merge tokenized Tooltip into main (#1434)

* Bring Tooltip to main

* remove extra space

* Bring changes from theming fix

* [XCUITests] Launch tests for all controls (#1461)

* add launch tests

* remove test template

* update to use base class + resolve comments

* change back to swift + use extension

* change back to base class with overridden control name

* resolve comments

* Add documentation for Avatar (#1469)

* add documentation for avatar

* add swiftui usage example

* Add documentation for Button (#1471)

* Added button documentation

* Fix table in documentation

* Fix table

* Rename table sections + fix path and typo

* Resized documentation images

* Cleanup

* More cleanup

* Added names to documentation

* fix tooltip png filenames (#1473)

* Update Date Picker test to start from `now` to fix test failure (#1477)

* Update Date Picker test to start from today

* Add explicit type interface

* Create a Readme section for demo app controls (#1268)

* Added new readme icon

* Added a vc for the readme

* Created scroll view and labels

* Added a ReadmeStrings class

* Removing title + polishing readme class

* Added a few more readmes

* Revert "Added new readme icon"

This reverts commit c73665f.

* Moving constant values into a struct

* Moved strings to their respective demo controllers

* Change from readme button text to icon

* Added todo to change color to fluent 2 ramp

* Moved constants struct to top of the file

* Use NSLayourConstraint.activate

* Added tvc readme section

* Removed unnecessary function

* Make fields private

* Refactor + added a few readmes

* Added more readmes

* Cleanup

* Move constraints logic to viewDidLoad

* Moved private properties to bottom of file

* Change unavailable to unavailable (#1475)

* Centralize missing readme string (#1479)

* [XCUITests] Add macOS launch tests (#1484)

* add macos launch tests

* try adding to ci

* remove other build commands

* change arch

* change macOS version

* add back build commands

* Resolving height issue (#1485)

* [XCUITests] Add iOS XCUITests to CI (#1480)

* test adding ios tests to ci

* remove -ios

* try changing to Demo.Development

* add new scheme

* add executable to run

* check run

* uncheck run

* change back to Demo.Development

* remove scheme

* change to just test

* add back scheme

* remove other build commands

* add back debug build

* add test retries

* comment out non tokenized tests

* remove retries + turn off parallelism

* add back other tests

* test if all are going to command bar

* change tests back to controlName

* add tear down that deletes app

* remove setup logic

* add timeouts

* change timeout to 1

* add back other build commands

* remove scheme

* whitespace change

* update tearDown to use isFirstLaunch

* fix linting issue

* remove isFirstLaunch from UserDefaults and add #if DEBUG

* try moving navigation back to setUpWithError

* whitespace change

* Skip failing date picker tests (#1495)

* Fix BadgeField space bug (#1497)

* attempt to fix space bug

* add comment

* [XCUITests] Activity Indicator tests (#1493)

* initial activity indicator tests

* add transition to dark mode

* whitespace change

* add dark mode color assert

* add default color to identifier

* update comment

* add dark mode + update to use .*

* add #if DEBUG

* add #if DEBUG to SceneDelegate

* add constants

* add type

* change to hex code

* removing color tests

* Revert "removing color tests"

This reverts commit 79c20c0.

* removing color tests again

* remove SceneDelegate change

* small change

* make code more readable + have SwiftUI inherit from UIKit

* remove super

* [XCUITests] Indeterminate Progress Bar tests (#1503)

* add indeterminate progress bar tests

* update SwiftUI to inherit from UIKit

* remove super

* resolve comments

* Resolving demo conflicts

* Resolving TableViewCell merge conflicts

* Taking tokenized tooltip from main - no fluent2

* Getting TVC building again

* Updating tooltip tokens

* Fixing TVC sample data and demo project

* Fixing token values

Co-authored-by: huwilkes <67026548+huwilkes@users.noreply.github.com>
Co-authored-by: Harrie Shin <hyshin@microsoft.com>
Co-authored-by: Sophia Lee <sophia.lee0416@gmail.com>
Co-authored-by: Jeanie Huynh <31874971+jeaniehuynh@users.noreply.github.com>
Co-authored-by: Joanna Qu <55368679+joannaquu@users.noreply.github.com>
Co-authored-by: Lamine Male <106181067+laminesm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants