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

Add CI tasks and scripts to run mobile performance tests #49547

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

derekblank
Copy link
Member

@derekblank derekblank commented Apr 3, 2023

What?

Adds workflows needed to run mobile performance tests via Reassure.

Why?

Native mobile performance tests have been added as part of #49221, and the results can be reported via CI to help identify performance regressions in comparison against a baseline measurement.

How?

Adds a shell script to be run via Github workflow.

Testing Instructions

N/A

Testing Instructions for Keyboard

N/A

@derekblank derekblank added Mobile App - Automation Label used to initiate Mobile App PR Automation Mobile App - Release PRs for native mobile editor releases labels Apr 3, 2023
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

Flaky tests detected in 3429b7e.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4613022939
📝 Reported issues:

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

This appears to have incorporated all of the feedback originally captured in #49221. 👍🏻

My perception is that this remains blocked by #49221 and work to configure the required GitHub token. Correct?

@derekblank
Copy link
Member Author

My perception is that this remains blocked by #49221 and work to configure the required GitHub token. Correct?

Correct. Once we have confirmed the CI token details, all PRs should be ready to review and merge.

@twstokes
Copy link
Contributor

👋 Hey @derekblank! Is this PR still blocked by the CI token issue? Thanks!

@derekblank
Copy link
Member Author

👋 Hey @derekblank! Is this PR still blocked by the CI token issue? Thanks!

Technically, yes. This change would allow Reassure test results to be reported via CI to Gutenberg, and would require an additional CI token. It is not pending any active apps infra request, however, as we de-prioritized this in pdnsEh-14N#comment-2105 to focus on more user-facing work. IMO, since Reassure test results can be viewed from the command line, this impact of this CI reporting change is low until more Reassure tests can be written and we can create a more realistic editor tree, at which point the impact of this change would be higher. When @dcalhoun returns, I would like to get his opinion on this as well.

@dcalhoun
Copy link
Member

dcalhoun commented Jun 9, 2023

This change would allow Reassure test results to be reported via CI to Gutenberg, and would require an additional CI token.

Given the decision to run automated tests within wordpress-mobile/gutenberg-mobile project, it may make sense to relocate bin/reassure-tests.sh to that repo. That would mean this PR could be closed in favor of wordpress-mobile/gutenberg-mobile#5579, which would become the sole PR blocked by the CI token that is unrelated to the WordPress/guteberg project. WDYT?

IMO, since Reassure test results can be viewed from the command line, this impact of this CI reporting change is low until more Reassure tests can be written and we can create a more realistic editor tree, at which point the impact of this change would be higher.

I agree the current, single test does not provide a lot of value. Once we add tests to cover newly fixed performance regressions, the Reassure tool begins showcasing its value, which would be amplified by running it on the CI server. I am not opposed to moving this work forward, but we may have more important work. That said, it likely would not hurt to inquire as to when we might receive a new token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - Automation Label used to initiate Mobile App PR Automation Mobile App - Release PRs for native mobile editor releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants