Skip to content

CI enable integration test on iOS Simulator #349

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

Merged
merged 17 commits into from
Apr 1, 2021

Conversation

sunmou99
Copy link
Contributor

@sunmou99 sunmou99 commented Mar 29, 2021

Script that running game-loop test on simulator: scripts/gha/test_simulator.py
A UI Test App that enable game-loop test: scripts/gha/integration_testing/gameloop

Workflow passed (admob,analytics):
https://github.com/firebase/firebase-cpp-sdk/runs/2241571055?check_suite_focus=true#step:33:28

Many test failed on simulator (due to keychain issue):
https://github.com/firebase/firebase-cpp-sdk/runs/2241582501?check_suite_focus=true#step:33:31

@google-cla google-cla bot added the cla: yes label Mar 29, 2021
@sunmou99 sunmou99 requested a review from jonsimantov March 29, 2021 21:13
@sunmou99 sunmou99 force-pushed the feature/test-ios-simulator branch 2 times, most recently from 819410f to ef4a0cc Compare March 29, 2021 22:40
Copy link
Contributor

@jonsimantov jonsimantov left a comment

Choose a reason for hiding this comment

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

What is this file? scripts/gha/integration_testing/gameloop.zip

@jonsimantov jonsimantov added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label Mar 30, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Mar 30, 2021
@sunmou99
Copy link
Contributor Author

What is this file? scripts/gha/integration_testing/gameloop.zip

I built the app & XCUITest, and compressed the test files into a .zip file.
Thus we could skip the build gameloop (app & test) step and save some time.

@sunmou99 sunmou99 requested a review from jonsimantov March 30, 2021 19:44
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Mar 30, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Mar 30, 2021
@jonsimantov
Copy link
Contributor

jonsimantov commented Mar 31, 2021

Rather than letting our tests go red, could you add a flag that turns off the simulator tests, so that we can file a ticket to re-enable and fix them?

(I recommend an env setting since we are out of input parameters.)

@jonsimantov jonsimantov added the tests-requested: quick Trigger a quick set of integration tests. label Mar 31, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. tests: failed This PR's integration tests failed. labels Mar 31, 2021
Copy link
Contributor

@vimanyu vimanyu left a comment

Choose a reason for hiding this comment

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

Minor comments on python code.

logging.info("No test Result")
return None

log_path = os.path.join(result.stdout.strip(), "Documents/GameLoopResults/Results1.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the log path guaranteed to be this all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Or we could iterate the directory to find the result.

"""Create a simulator locally. Will wait until this simulator botted."""
device_info = ios_device.split("-")
device_name = device_info[0]
device_os = device_info[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

This could error if the device name does not have a "-". Maybe check for that first and raise exception if that is the case?
A comment stating an example string that could be passed for ios_device will be helpful to follow any string operations.

Copy link
Contributor Author

@sunmou99 sunmou99 Mar 31, 2021

Choose a reason for hiding this comment

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

I comment here:

I checked before, no device name (both Android & iOS) contains "-".
But I noticed that setting device info could be a problem.
My next plan is to expend the test device matrix. And I will figure out how to deal with it then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant device_info could be missing a "-". and the token split will fail in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Currently, we only use the default value, thus it won't be a problem.
But I will keep this in mind when expending the test device matrix.

ios_device = FLAGS.ios_device

config_path = os.path.join(current_dir, "integration_testing/build_testapps.json")
with open(config_path, "r") as config:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: file handle variable could be named "config_file" for easier reading. Json data and file handle have the same name currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Historically, testing scripts name variable this way.

import XCTest

/// Constants used across the loop launcher and UI test.
public struct Constants {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know much about swift but noticed the same constants are also defined in "Constants.swift". Could we reuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I create this class for reuse.
But sadly, UITest focus on UI, and cannot get access to the app's source code.
AFAIK, the way to reuse this class is to build it into a Framework.

@jonsimantov
Copy link
Contributor

What is this file? scripts/gha/integration_testing/gameloop.zip

I built the app & XCUITest, and compressed the test files into a .zip file.
Thus we could skip the build gameloop (app & test) step and save some time.

I don't really like including a prebuilt app in our repo - is this zip file used by the script? Or does the script build the app?

@sunmou99
Copy link
Contributor Author

What is this file? scripts/gha/integration_testing/gameloop.zip

I built the app & XCUITest, and compressed the test files into a .zip file.
Thus we could skip the build gameloop (app & test) step and save some time.

I don't really like including a prebuilt app in our repo - is this zip file used by the script? Or does the script build the app?

The script uses this zip file

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Mar 31, 2021
@sunmou99 sunmou99 requested a review from vimanyu March 31, 2021 21:11
@sunmou99
Copy link
Contributor Author

Rather than letting our tests go red, could you add a flag that turns off the simulator tests, so that we can file a ticket to re-enable and fix them?

(I recommend an env setting since we are out of input parameters.)

Add an env, and modified the build script to support it.
Also, test device takes 4 inputs. I think is better to address this issue in the following expending test device matrix ticket.

@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Mar 31, 2021
@sunmou99 sunmou99 added the tests-requested: quick Trigger a quick set of integration tests. label Mar 31, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. tests: failed This PR's integration tests failed. labels Mar 31, 2021
@github-actions
Copy link

github-actions bot commented Mar 31, 2021

❌  Integration test FAILED

Requested by @sunmou99 on commit 79affe1
Last updated: Wed Mar 31 18:52:52 PDT 2021
View integration test results

Platform Build failures Test failures
iOS (built on MacOS) database

@sunmou99 sunmou99 enabled auto-merge (squash) April 1, 2021 01:32
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Apr 1, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 1, 2021
@sunmou99 sunmou99 removed the tests: failed This PR's integration tests failed. label Apr 1, 2021
@sunmou99 sunmou99 merged commit c74dc47 into main Apr 1, 2021
@sunmou99 sunmou99 deleted the feature/test-ios-simulator branch April 1, 2021 01:55
@firebase firebase locked and limited conversation to collaborators May 2, 2021
@sunmou99 sunmou99 changed the title Add test on iOS simulator CI enable integration test on iOS simulator Feb 25, 2022
@sunmou99 sunmou99 changed the title CI enable integration test on iOS simulator CI enable integration test on iOS Simulator Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants