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

feat: add building espresso server command via appium command #858

Merged
merged 15 commits into from
Mar 22, 2023

Conversation

KazuCocoa
Copy link
Member

This is still wip. I'd like to extract the building espresso server via npm command over appium command, eventually.
https://github.com/appium/appium-espresso-driver#espresso-build-config

This does not change the gradle file itself, but it allows users to build the server with some parameters #851

@KazuCocoa KazuCocoa changed the title [wip] add espresso build command feat: add building espresso server command via appium command Mar 20, 2023
@KazuCocoa KazuCocoa marked this pull request as ready for review March 20, 2023 01:43
await builder.build();

const dstPath = path.resolve(ESPRESSO_SERVER_ROOT, 'app', 'build', 'outputs', 'apk', 'androidTest', 'debug');
console.log(`The built server apk, app-debug-androidTest.apk, is in ${dstPath}`); // eslint-disable-line no-console
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather just show the full path together with the file name

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered the way as well, but I expected the primary usage was to open the path with Finder (on macOS for example) to refer to the apk instead of opening the apk itself.

Some terminal tools can open the path directly. e.g. cmd + click the link. When they open the path, if the path is .apk, the system will open an application instead of the Finder. Some tools require selecting the entire path and copy&paste it. This does not have much differences in the full path vs the file's directory.

Thus, at this time, showing the directory helps the expected use case with some tools, but other tools do not have differences to reach the .apk. So I did the current method.

I like the current way for this use case I expected, but I can put the full path instead.

}

const dstPath = path.resolve(ESPRESSO_SERVER_ROOT, 'app', 'build', 'outputs', 'apk', 'androidTest', 'debug');
LOG.info(`The built server apk, app-debug-androidTest.apk, is in ${dstPath}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the dstPath values should already contain app-debug-androidTest.apk

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, I've changed. Still I like to open the directory to open the link via cmd+click in a terminal, but no strong opinion for this.


if (process.env.ESPRESSO_BUILD_CONFIG) {
if (!(await fs.exists(process.env.ESPRESSO_BUILD_CONFIG))) {
throw Error(`'${process.env.ESPRESSO_BUILD_CONFIG}' did not exist. Please set the path as an absolute path.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

new Error, same below

opts.buildConfiguration = JSON.parse(buildConfigurationStr);
LOG.info(`The espresso build config is ${JSON.stringify(opts.buildConfiguration)}`);
} catch (e) {
throw Error(`Failed to parse the ${process.env.ESPRESSO_BUILD_CONFIG}. Please make sure that the JSON is valid format. Error: ${e}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

these lines are too long. Please split them. Same below


const dstPath = path.resolve(ESPRESSO_SERVER_ROOT, 'app', 'build', 'outputs', 'apk', 'androidTest', 'debug', 'app-debug-androidTest.apk');
if (await fs.exists(dstPath)) {
LOG.info(`The built server apk is ${dstPath}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather say: Full path to the server APK: ...

if (await fs.exists(dstPath)) {
LOG.info(`The built server apk is ${dstPath}`);
} else {
LOG.info(`The built server androidTest.apk might be in ${ESPRESSO_SERVER_BUILD}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Full path to the server build folder: ...

README.md Outdated
@@ -42,7 +42,12 @@ On top of standard Appium requirements Espresso driver also expects the followin

## Scripts

- `appium driver run print-espresso-path` prints the path to the Appium Espresso server root. You can modify the gradle file directly if [Espresso Build Config](#espresso-build-config) was not sufficient.
- `appium driver run espresso print-espresso-path` prints the path to the Appium Espresso server root. You can modify the gradle file directly if [Espresso Build Config](#espresso-build-config) was not sufficient.
- `appium driver run espresso build-espresso` builds the espresso server since driver version 2.18.0. It helps building the espresso server outside Appium process. Available environment variables are below:
Copy link
Contributor

Choose a reason for hiding this comment

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

outside of the Appium process

@KazuCocoa KazuCocoa merged commit cfaad9a into appium:master Mar 22, 2023
@KazuCocoa KazuCocoa deleted the kazucocoa/espresso-server-build branch March 22, 2023 08:23
github-actions bot pushed a commit that referenced this pull request Mar 22, 2023
## [2.18.0](v2.17.0...v2.18.0) (2023-03-22)

### Features

* add building espresso server command via appium command ([#858](#858)) ([cfaad9a](cfaad9a))
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.

2 participants