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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automated framed screenshots #28

Merged
merged 9 commits into from Apr 18, 2018
Merged

Conversation

mkchoi212
Copy link
Collaborator

@mkchoi212 mkchoi212 commented Apr 1, 2018

Major feat in this PR are

  • Support for fastlane screenshots with localization
  • New UI tests for the new shiny UI with localization

I wasn't sure which screens to capture and what kind of text to put in the frames but here's what I came up with. Here's some demo screenshots in German; thanks to Google Translate 馃槂

Please note that code for screenshooting is part of UI testing

@mkchoi212
Copy link
Collaborator Author

circleci seems to be getting the following error, which I'm not getting on my local machine

鉂  /Users/distiller/project/VLC for iOSUITests/VLCiOSTestVideoCodecs.swift:13:8: cannot load underlying module for 'XCTest'
[00:21:20]: 鈻 import XCTest

// - Audio tab home
// - Local Network tab home
// - Video tab home
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

This header comment does not follow the format of the project, specially regarding the license part. Please check an existing file as reference.


override func tearDown() {
super.tearDown()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this method be removed?

//
// Screenshots taken
// - Video playback (Jellyfish)
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, this header comment does not follow the project format


override func tearDown() {
super.tearDown()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, I think this method could be removed

//
// Created by Mike Choi on 3/30/18.
// Copyright 漏 2018 VideoLAN. All rights reserved.
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

鈽濓笍 馃

Podfile Outdated
@@ -23,6 +23,7 @@ def iOS_pods
pod 'MediaLibraryKit-prod'
pod 'MobileVLCKit', '3.0.2'
pod 'GTMAppAuth'
pod 'SimulatorStatusMagic', :configurations => ['Debug']
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is adding the dependency to the main application targets increasing their bundle sizes. Could it be possible to define it only in the uitests target?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving pod 'SimulatorStatusMagic' to target 'VLC for iOSUITests' won't allow VLCAppDelegate.mto import the module, right?

https://github.com/mkchoi212/vlc-ios/blob/6383eee5f46849f493a4ee29e6b7e9224f2c0622/Sources/VLCAppDelegate.m#L38

Copy link
Member

@carolanitz carolanitz Apr 2, 2018

Choose a reason for hiding this comment

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

correct! @dcordero correct me if I'm wrong here please.
I think instead of importing it directly you have an extension on Appdelegate for the tests and that extension is only part of the testmodule. You would also not need the debug configuration that way. Though I'm not sure how that works with classmethods. I would take a look at Artsys App here

@dcordero
Copy link
Collaborator

dcordero commented Apr 1, 2018

Great work, the screenshots look awesome !!!

I have added a few comments here and there

@dcordero
Copy link
Collaborator

dcordero commented Apr 2, 2018

The error in circleci means that XCTest is been imported from an Application target. Are the those new swift files included in the test target or the app target? That could be the reason of the error.

Copy link
Member

@carolanitz carolanitz left a comment

Choose a reason for hiding this comment

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

This looks already soooo great!! I had some inline comments. I can't wait until this is ready to be merged! :D
Btw for screenshotting we got the permission to use the following files :
https://www.youtube.com/watch?v=ba62uuv-5Dc
https://www.youtube.com/watch?v=6NDYf3RZ1eY
I'm not sure how this would work exactly but there is a way to have these videos (after downloading them from youtube)in the User Directory. That way we actually have files in the Video and Audio tab respectively. This should probably be part of an additional PR though :)

.gitignore Outdated
@@ -10,7 +10,6 @@ xcuserdata
# Fastlane
fastlane/report.xml
fastlane/Preview.html
fastlane/screenshots
Copy link
Member

Choose a reason for hiding this comment

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

According to the fastlane docs it's recommended to not store the screenshots in the repository
https://docs.fastlane.tools/best-practices/source-control/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will make modifications based on this guide! 馃憤

Copy link
Member

Choose a reason for hiding this comment

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

awesome that's even better! Great find :)

Podfile Outdated
@@ -23,6 +23,7 @@ def iOS_pods
pod 'MediaLibraryKit-prod'
pod 'MobileVLCKit', '3.0.2'
pod 'GTMAppAuth'
pod 'SimulatorStatusMagic', :configurations => ['Debug']
Copy link
Member

@carolanitz carolanitz Apr 2, 2018

Choose a reason for hiding this comment

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

correct! @dcordero correct me if I'm wrong here please.
I think instead of importing it directly you have an extension on Appdelegate for the tests and that extension is only part of the testmodule. You would also not need the debug configuration that way. Though I'm not sure how that works with classmethods. I would take a look at Artsys App here


class VLCiOSTestMenu: XCTestCase {
let app = XCUIApplication()
let moreTab = XCUIApplication().tabBars.buttons.element(boundBy: 4)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to get the tab by localized title instead ?

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Apr 2, 2018

Choose a reason for hiding this comment

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

There is no localizable string for this :(
I think the "More" tab is using Apple's built-in localized strings.

}

func localizedString(key: String) -> String {
let localizationBundle = Bundle(path: Bundle(for: VLCiOSTestMenu.self).path(forResource: deviceLanguage, ofType: ".lproj")!)!
Copy link
Member

Choose a reason for hiding this comment

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

force unwrap is not great. How do you feel about making the tests fail if the localization file can't be loaded?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally it should be enough to load the bundle once for every language. That would speed up the test

}

func testNavigationToVideoTab() {
app.tabBars.buttons["Video"].tap()
Copy link
Member

Choose a reason for hiding this comment

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

Is there no localizable string ?

func localizedString(key: String) -> String {
let localizationBundle = Bundle(path: Bundle(for: VLCiOSTestMenu.self).path(forResource: deviceLanguage, ofType: ".lproj")!)!
let result = NSLocalizedString(key, bundle: localizationBundle, comment: "")
return result
Copy link
Member

Choose a reason for hiding this comment

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

seeing that this is used here as well it might be even good to have a testhelpers method ? We should definitely not duplicate the code here

@@ -0,0 +1,13 @@
devices([
"iPhone X",
"iPhone 8"
Copy link
Member

Choose a reason for hiding this comment

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

I think we will need all the devices :D

languages([
"en",
"de",
"fr",
Copy link
Member

Choose a reason for hiding this comment

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

we will also need all the languages that we support. And yes I know this will take forever

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To create the framed screenshots, we need localized keyword.string and title.string for each language. Maybe this should be a progressive effort once we officially decide on what text to put on the framed screenshots??

Copy link
Member

Choose a reason for hiding this comment

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

absolutely let's just ask Louis!

}
}

// Please don't remove the lines below
Copy link
Member

Choose a reason for hiding this comment

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

I'm expecting that you double checked and understood this file btw :)

Copy link
Member

@carolanitz carolanitz left a comment

Choose a reason for hiding this comment

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

we should switch to the accessibility element identifiers. that will resolve the bundle and localization issues. Also the simulator magic only as part of the Testtarget and adding it in the setup for the tests should resolve the import conundrum

XCUIDevice.shared.orientation = .portrait
setupSnapshot(app)
helper = TestHelper(lang: deviceLanguage, target: VLCiOSTestMenu.self)
app.launch()
Copy link
Member

Choose a reason for hiding this comment

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

This here would actually be a better place for the status magic (but remember to disable it again in tearDown()).

import XCTest

struct TestHelper {
let localizationBundle: Bundle
Copy link
Member

Choose a reason for hiding this comment

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

something that I didn't think about in my earlier review but that a friend pointed out is to use accessibility element identifiers. They don鈥檛 need to be localized! Since we need to make the app accessibility friendly anyway this will kill two birds with one stone :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it looks like none of the elements in the app has its accessibilityIdentifier set. How should we go about this? Update every single element's accessibilityIdentifier?

Copy link
Member

Choose a reason for hiding this comment

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

yes exactly. It needs to be added

helper.tap(tabDescription: audio, app: app)
XCTAssertNotNil(app.navigationBars[audio])

snapshot("audio_tab")
Copy link
Member

Choose a reason for hiding this comment

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

btw could we separate the Unit Tests from the snapshot taking?

@mkchoi212 mkchoi212 force-pushed the master branch 2 times, most recently from d5bb0a6 to 6956146 Compare April 4, 2018 02:24
.gitignore Outdated
fastlane/test_output

fastlane/screenshots/*/*.png
fastlane/screenshots/screenshots.html
Copy link
Member

@carolanitz carolanitz Apr 4, 2018

Choose a reason for hiding this comment

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

will you hate me if I ask for an alphabetically ordered .gitignore 馃檴? At least within the fastlane section

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not at all 馃槃

@mkchoi212 mkchoi212 force-pushed the master branch 3 times, most recently from 7276bcc to 8978eaf Compare April 7, 2018 01:05
@carolanitz
Copy link
Member

looks already better. Could you use the accessibilityidentifier on the tabbaritems and other views as well? We need to avoid all use of the localization bundle otherwise renaming a tab would lead to failing tests for no reason

@carolanitz
Copy link
Member

There is also something misconfigured in the xcodeproj for me. Might be just me though screen shot 2018-04-09 at 2 16 01 pm

"iPad Pro (12.9-inch)",
"iPad Pro (12.9-inch) (2nd generation)",
"iPad Pro (10.5-inch)"
])
Copy link
Member

Choose a reason for hiding this comment

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

馃憤

@carolanitz
Copy link
Member

the tests fail on iPad btw but the more tab will disappear anyway soon. And one more question. What are the screenshots doing for a language that is not defined? can we make sure that it uses the screenshots from the English version?

@mkchoi212
Copy link
Collaborator Author

@carolanitz Right now, frameitdoes not work with any language that does not have their respective .string files. But let me see if I can make the default language English for them!

@mkchoi212
Copy link
Collaborator Author

Please note that because the "More" tab is going to be removed soon, the test does not take the "More" tab into consideration. Because of this, the tests fail for all iPhones but succeed for all iPads

@mkchoi212 mkchoi212 force-pushed the master branch 4 times, most recently from 83c49b1 to d25f37f Compare April 12, 2018 02:04
@carolanitz
Copy link
Member

the merge conflicts just need to be resolved and I think this is good to go

@mkchoi212 mkchoi212 force-pushed the master branch 2 times, most recently from 7036aee to 5b19e9c Compare April 13, 2018 16:30
@mkchoi212
Copy link
Collaborator Author

Fixed all the merge conflicts!
Sorry for such a huge PR but thanks for the comments @dcordero @carolanitz 鉂わ笍
I'll make sure to break up these bad boys into smaller chunks next time!

Perfect status bar displays 100% battery life and 9:41 as the time.
This allows for consistent, and perfect screenshoting, 100% of the time
To take framed screenshots, run `fastlane screenshots`
Note that the list of supported devices and languages is random for now.
Notice that screenshots for fastlane are taken in the test file
by calling `snapshot("IMG_NAME")`
Note that only German, French, and English have been added so far.
Design/content for the framed screenshots are BASIC and NEEDS
IMPROVEMENT.
iOS test cases are now compatible with both iPhone and iPad.
Note that a new target has been created in the Podfile specifically for
iOS UITests.
Fixed bugs regarding change of localization during screenshoting
Note that string literals are being used all over the project to create
identifiers. A new file containing all identifier literals should be
created in the future.
Fastlane does not support default langauges.
Therefore, in order to generate screenshots for all languages, default
English keyword.strings and title.strings have been copied for all
langauges.
@mkchoi212
Copy link
Collaborator Author

All commits have been squashed!

@@ -3431,7 +3408,6 @@
ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES;
CLANG_ANALYZER_NONNULL = YES;
CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION = YES_AGGRESSIVE;
CLANG_CXX_LANGUAGE_STANDARD = "gnu++14";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@carolanitz, this fixes the misconfigured in the xcodeproj problem that you discovered!

@vlc-mirrorer vlc-mirrorer merged commit e62a7fb into videolan:master Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants