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

Bugfix MTE-4133 Enable retries in fastlane #24251

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

clarmso
Copy link
Collaborator

@clarmso clarmso commented Jan 20, 2025

📜 Tickets

Jira ticket

💡 Description

Let's enable retries on the L10n test plan so that we will get some screenshots in just in case of flaky tests.

Bitrise L10nBuild workflow: https://app.bitrise.io/build/f078a5ac-5f28-4a23-b444-6fd7238d012f?tab=log

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@clarmso clarmso requested a review from a team as a code owner January 20, 2025 19:46
@@ -46,7 +46,7 @@ for lang in $LOCALES; do
if [ "$?" != "0" ]; then
echo "Fastlane exited with code: $?"
exit $?
elif grep -q "** TEST FAILED **"; then
elif grep -q "TEST FAILED" "output.txt"; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"output.txt" was missing previously, so this line might not be working properly before.

@@ -34,7 +34,7 @@ for lang in $LOCALES; do
echo "$(date) Snapshotting $lang"
mkdir "l10n-screenshots/$lang"
fastlane snapshot --project firefox-ios/Client.xcodeproj --scheme L10nSnapshotTests \
--number_of_retries 0 \
--number_of_retries 3 \
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 can't tell if the default value has changed or not by reading the documentation. Let's hard code this value.

Copy link
Collaborator Author

@clarmso clarmso Jan 20, 2025

Choose a reason for hiding this comment

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

👀 testHistoryTableContextMenu failed. The whole list of tests are rerun. This may not be what we want.
https://app.bitrise.io/build/bc7e1954-3780-4157-afeb-51e10ad9ff3d?tab=log

Copy link
Contributor

@isabelrios isabelrios Jan 21, 2025

Choose a reason for hiding this comment

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

no, we don't want retries. Just one execution per locale no matter the reesult. Please see explanation here:https://mozilla.slack.com/archives/C02KSH6QNBS/p1737374666180979?thread_ts=1737374476.241319&cid=C02KSH6QNBS

Copy link
Contributor

Choose a reason for hiding this comment

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

This option is ignored now by fastlane. We should use set the retry opion to off from xcode and to do that, we need to create a test plan for the L10n schema where we can configure that. I have done that locally, forced a test to fail, and there are no retries.
Please try that and let me know in case you want a hack session to do that together.

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 have created a L10nSnapshotTests test plan and have the schema to use the test plan.

--skip_open_summary \
--xcargs "-maximum-parallel-testing-workers 2" \
--derived_data_path l10n-screenshots-dd \
--ios_version "18.2" \
--erase_simulator --localize_simulator \
--devices "iPhone 16" --languages "$lang" \
--output_directory "l10n-screenshots/$lang" \
--xcodebuild_formatter xcbeautify \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default formatter on Bitrise is now xcbeautify. I have intermittent issues on xcpretty currently with the latest fastlane. Let me ensure that I use the same formatter locally.

}
],
"defaultOptions" : {
"testRepetitionMode" : "retryOnFailure",
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 have enabled "Retry on Failure" for the test plan's config. The default value is 3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I run the test on Xcode 16, I see the following line in the logs:

Test Case '-[L10nSnapshotTests.L10nSuite1SnapshotTests test3ReloadButtonContextMenu]' started (Iteration 1 of 3).

In addition, a test retries 3 times when I made it to fail on purpose.

Copy link
Contributor

@isabelrios isabelrios Jan 22, 2025

Choose a reason for hiding this comment

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

we should NOT retry on failure. Please read my comments above and on slack about that.
The option --number_of_retries 0 is being ignored. By adding a test plan what we want is to have the option to set the retry to off in xcode. It is being ignored with the fastlane option but looks like if we have it enabled via xcode it works.
We don't want tests to retry because we run 6 locales per build, if there are retries, it takes too long to run each set of locales that builds timeout in Bitrise. If a test fail, we only miss that screenshoot, the rest are generated

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