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

WALLET-412: Run UI-based tests in CI #20

Merged
merged 31 commits into from
Aug 20, 2024
Merged

Conversation

NSeydoux
Copy link
Contributor

@NSeydoux NSeydoux commented Aug 8, 2024

This adds the setup required to run the Detox UI-based tests in CI:

  • A Dockerfile with the Android SDK and emulator
  • A Github Action running on push to run the tests. (not done yet)

@NSeydoux NSeydoux force-pushed the chore/WALLET-412/detox-in-ci branch from 730b426 to 86cda4e Compare August 12, 2024 08:31
@NSeydoux
Copy link
Contributor Author

The tests run correctly on my local machine, but the app crashes when I try running it in Docker.
test

@NSeydoux NSeydoux marked this pull request as ready for review August 14, 2024 12:28
@NSeydoux NSeydoux requested review from a team as code owners August 14, 2024 12:28
@NSeydoux
Copy link
Contributor Author

This is ready for review, as it adds a couple of changes that are desirable:

  • Using a simpler API for WebView tests
  • Test output gets persisted, with logs, video and screenshots
  • The signing configuration no longer requires a change in the global gradle config, which makes it easier to integrate in CI/CD

However, the main point of this work was to enable UI tests in CI, or at least in Docker, and that didn't work out.

.detoxrc.js Outdated
Comment on lines 77 to 89
artifacts: {
rootDir: process.env.NODE_ENV === "docker" ? "/screenshots/" : "./screenshots/",
plugins: {
log: {"enabled": true},
uiHierarchy: {"enabled": true},
screenshot: {
keepOnlyFailedTestsArtifacts: false,
takeWhen: {
"testStart": true,
"testDone": true
}
},
video: {
enabled: true
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds more output to the UI-based tests, which is useful for debug purpose even outside of CI.

Dockerfile Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Dockerfile is a basis, but it currently does not run the UI-based tests successfully. Arguably, this could be removed from the repo altogether, but some of the work that went in running the emulator in the container is good to keep around I would say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change aligns the naming convention with the gradle typical system properties.

Comment on lines -61 to +62
await this.signInFormUsernameInput.runScript(`(element) => {
element.value = "${username}";
}`);
await this.signInFormUsernameInput.replaceText(username);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels like a more readable approach, using a more appropriate API for the task.

});

await expect(this.signInSubmitButton).toExist();
await this.signInSubmitButton.tap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto using a more expressive API for the given task.

Comment on lines -26 to +29
by.web.xpath('//button[text()="Continue"]')
by.web.cssSelector('button[data-testid="prompt-continue"]')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using data-testid attributes, meant for automated tests and more stable than text content, is a good practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change moves the signing configuration out of the global gradle config into the environment, which is easier to integrate in CI/CD and is a more appropriate scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the Dockerfile, useful findings but currently not working as desired.

Copy link
Contributor

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

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

There are some conditionals based on running docker in CI and there are still instructions for docker in the readme. Do these need cleaning up now we are not including the dockerfile?

This is an initial test to see if hardware acceleration works in CI
Previously, the keystore configuration was stored in the global gradle
configuration, which is fine for dev machines, but more problematic for
CI. Keystore configuration now comes from environment variables, which
is easier to manipulate in CI.
HW acceleration is available in the CI runner.
@NSeydoux NSeydoux force-pushed the chore/WALLET-412/detox-in-ci branch from 132293d to e235929 Compare August 20, 2024 08:16
Copy link
Contributor

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

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

LGTM

@edwardsph edwardsph merged commit 363ae0d into main Aug 20, 2024
7 checks passed
@edwardsph edwardsph deleted the chore/WALLET-412/detox-in-ci branch August 20, 2024 08:27
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