Enable Core Data concurrency debug option in UI tests by crazytonyli · Pull Request #20395 · wordpress-mobile/WordPress-iOS

crazytonyli · 2023-03-24T00:22:23Z

The app crashes during UI tests when a Core Data concurrency issue occurs.

A Core Data concurrency issue is typically where a managed object is accessed outside of the context (or the queue managed by the context). Currently there are still many such issues in the app, post editing being the most impacted one, which is why the new
crashOnCoreDataConcurrencyIssue option is disabled in EditorGutenbergTests.

But, I think it's still valuable to ensure that those areas that don't have any Core Data concurrency issues to be maintained. And we should expose the new issue if one does occur. Doing that in UI tests is one easy way I can think of, which doesn't have a "blocking" factor—we can always disable the check for specific test(s).

Regression Notes

N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-03-24T01:39:17Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr20395-46f6655
	Version	22.0
	Bundle ID	org.wordpress.alpha
	Commit	`46f6655`
	App Center Build	WPiOS - One-Offs #5387

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-03-24T01:41:11Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr20395-46f6655
	Version	22.0
	Bundle ID	com.jetpack.alpha
	Commit	`46f6655`
	App Center Build	jetpack-installable-builds #4415

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio

The UI tests on iPhone had 26 failures (waiting for #20378 to have better reporting on flakiness). I wonder how many of those were due to Core Data issues 🤔

If there are many, I'd worry they would introduce too much noise in the report—Unless, that is, we committed to fix those issues ASAP.

mokagio · 2023-03-24T04:46:25Z


+        if crashOnCoreDataConcurrencyIssues {
+            app.launchArguments.append(contentsOf: ["-com.apple.CoreData.ConcurrencyDebug", "1"])
+        }


Only a subset of the tests use setUpTestSuite, which would result in this argument being applied inconsistently.

Have you considered using the test plan to set it and use this method to opt-out only?

Good suggestion. I hadn't thought about it, but I'll give it a go.

Only a subset of the tests use setUpTestSuite

The idea is only turn the debug option on for UI tests, which all calls this funciton, I think?

[UI tests] all calls this funciton, I think?

Oh, that is correct. I don't know why I got the impression only some of them called setUpTestSuite 🤔 But I checked each test and they all do.

Apologies.

crazytonyli · 2023-03-24T06:59:32Z

The UI tests on iPhone had 26 failures

Sorry, I should've mentioned in the PR description. All those failures should be addressed by #20394. I'll wait for that PR to be merged to re-run all the UI tests in this PR.

[...] have better reporting on flakiness

I don't think this change would cause flaky tests. I can later try re-run UI tests a few times to be sure.

mokagio · 2023-03-28T00:38:01Z

@crazytonyli I see #20394 is not in this branch via 66b4efc. However, we still have more UI tests failures than other branches 🤔

crazytonyli · 2023-03-29T01:52:29Z

I think the UI tests failed because they caught a known Core Data concurrency issue which should be fixed by #20413. Unfortunately, the CI job doesn't tell us the failure reason. The result bundle just says "timeout" because the app crashed. So, it would be good to attach a crash report to the result bundle if that's possible.

mokagio

Looks like all the tests failures we saw initially have been addressed by the PRs, as expected.

crazytonyli · 2023-04-02T21:33:49Z

I can later try re-run UI tests a few times to be sure.

Buildkite doesn't allow retrying a successful step. I think I'll have to amend the git history to re-build, which is what I'm going to do, for a few times, to be sure that this change won't cause UI test becoming flaky.

The app crashes during UI tests when a Core Data concurrency issue occurs. A Core Data concurrency issue is typically where a managed object is accessed outside of the context (or the queue managed by the context). Currently there are still many such issues in the app, post editing being the most impacted one, which is why the new `crashOnCoreDataConcurrencyIssue` option is disabled in `EditorGutenbergTests`. But, I think it's still valuable to ensure that those areas that don't have any Core Data concurrency issues to be maintained. And we should expose the new issue if one does occur. Doing that in UI tests is one easy way I can think of, which doesn't have a "blocking" factor—we can always disable the check for specific test(s).

peril-wordpress-mobile · 2023-04-02T23:50:56Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

mokagio · 2023-04-03T01:01:49Z

Buildkite doesn't allow retrying a successful step. I think I'll have to amend the git history to re-build, which is what I'm going to do, for a few times, to be sure that this change won't cause UI test becoming flaky.

I opened #20461 to run the UI tests step 6 times, which should increase our confidence.

I'm also going to bump this milestone to 22.2 as I'm about to code freeze 22.1 and this is not a user facing PR so it doesn't matter whether it lands in 22.1 now or on trunk after the code freeze.

crazytonyli · 2023-04-03T01:02:02Z

The CI jobs have been passing three times in a row, which I'll take as a confirmation that this PR doesn't cause flaky UI tests and merge this PR.

mokagio · 2023-04-03T01:05:41Z

@crazytonyli race condition 😄

Anyway, good choice. I should have checked to see if theres was something running in CI before going ahead and starting my experiment. I'll cancel it, then.

crazytonyli · 2023-04-03T02:30:55Z

@mokagio Ops, sorry. That's a way smarter approach than my force-push 👍

crazytonyli added the Core Data Issues related to Core Data label Mar 24, 2023

crazytonyli added this to the 22.1 milestone Mar 24, 2023

crazytonyli requested a review from mokagio March 24, 2023 00:22

crazytonyli self-assigned this Mar 24, 2023

crazytonyli requested a review from a team as a code owner March 24, 2023 00:22

crazytonyli force-pushed the enable-core-data-concurrency-debug-option-in-ui-tests branch from 0074f2b to 240cd08 Compare March 24, 2023 01:53

mokagio reviewed Mar 24, 2023

View reviewed changes

crazytonyli force-pushed the enable-core-data-concurrency-debug-option-in-ui-tests branch from 66b4efc to 87892b5 Compare March 30, 2023 08:18

mokagio approved these changes Mar 30, 2023

View reviewed changes

crazytonyli force-pushed the enable-core-data-concurrency-debug-option-in-ui-tests branch from 87892b5 to cd00c76 Compare April 2, 2023 21:34

crazytonyli force-pushed the enable-core-data-concurrency-debug-option-in-ui-tests branch from cd00c76 to 46f6655 Compare April 2, 2023 23:50

mokagio modified the milestones: 22.1, 22.2 Apr 3, 2023

crazytonyli merged commit f316bf4 into trunk Apr 3, 2023

crazytonyli deleted the enable-core-data-concurrency-debug-option-in-ui-tests branch April 3, 2023 01:02

mokagio modified the milestones: 22.2, 22.1 Apr 3, 2023

mokagio mentioned this pull request Apr 3, 2023

PR to "stress test" #20395 #20461

Closed

Uh oh!

Conversation

crazytonyli commented Mar 24, 2023

Regression Notes

Uh oh!

wpmobilebot commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented Mar 24, 2023

Uh oh!

mokagio commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazytonyli commented Mar 29, 2023

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented Apr 2, 2023

Uh oh!

peril-wordpress-mobile Bot commented Apr 2, 2023

Uh oh!

mokagio commented Apr 3, 2023

Uh oh!

crazytonyli commented Apr 3, 2023

Uh oh!

mokagio commented Apr 3, 2023

Uh oh!

crazytonyli commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wpmobilebot commented Mar 24, 2023 •

edited

Loading

wpmobilebot commented Mar 24, 2023 •

edited

Loading

mokagio commented Mar 28, 2023 •

edited

Loading