Skip to content

Extract performance tests - #22287

Closed
kean wants to merge 5 commits into
trunkfrom
task/disable-slow-performance-tests
Closed

Extract performance tests#22287
kean wants to merge 5 commits into
trunkfrom
task/disable-slow-performance-tests

Conversation

@kean

@kean kean commented Dec 25, 2023

Copy link
Copy Markdown
Contributor

Implements the suggestion from "RFC – Dedicated test suite for slow and performance tests".

Adds a new target to run the performance tests. I used WordPress as a target app and try to keep the target configuration as simple as possible. I ended up duplicating a bit of code, which I think is acceptable in this case.

I suggest not running the performance tests on the CI, as there is no good way to compare the performance across different executions on difference machines. The performance tests are mainly useful when working on optimizations to compare the before and the after.

To test:

  • Run pod install
  • Verify that the new WordPressPerformanceTests target runs

Measuring the impact:

// Before
Test Suite 'All tests' failed at 2023-12-25 14:55:09.438.
Executed 1922 tests, with 1 test skipped and 21 failures (0 unexpected) in 77.174 (78.079) seconds

// After
Test Suite 'All tests' failed at 2023-12-25 13:28:16.761.
Executed 1918 tests, with 1 test skipped and 21 failures (0 unexpected) in 54.920 (55.530) seconds

That's a 22.5 sec improvement and with the removal of the outdated version, the unit test execution time will be reduced by nearly half.

Regression Notes

  1. Potential unintended areas of impact: n/a
  2. What I did to test those areas of impact (or what existing automated tests I relied on): n/a
  3. What automated tests I added (or what prevented me from doing so): 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.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean
kean requested review from crazytonyli and mokagio December 25, 2023 14:55
@kean kean added Testing Unit and UI Tests and Tooling [Type] Task labels Dec 25, 2023
@kean kean added this to the Pending milestone Dec 25, 2023
@kean kean changed the title RFC: Extract performance tests Extract performance tests Dec 25, 2023
@wpmobilebot

wpmobilebot commented Dec 25, 2023

Copy link
Copy Markdown
Contributor
WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22287-d9231a3
Version23.9
Bundle IDorg.wordpress.alpha
Commitd9231a3
App Center BuildWPiOS - One-Offs #8236
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot

wpmobilebot commented Dec 25, 2023

Copy link
Copy Markdown
Contributor
Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22287-d9231a3
Version23.9
Bundle IDcom.jetpack.alpha
Commitd9231a3
App Center Buildjetpack-installable-builds #7260
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean
kean marked this pull request as draft December 25, 2023 15:26
@kean
kean marked this pull request as ready for review December 25, 2023 19:57
@crazytonyli

Copy link
Copy Markdown
Contributor

With the changes in #22345, the PagesListTests now runs relatively fast. Do you think we still need to extract them out?

Here is the result on my M1 Max MacBook Pro
Screenshot 2024-01-09 at 11 47 45 AM

@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Jan 9, 2024
@kean

kean commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

I agree with @mokagio on this one:

I offer sub-decisecond as a target. That is, each individual test() in an XCTestCase subclass should run in less than 0.1 of a second.

I think that performance tests are not unit tests and hence need to be in a separate target, similar to the UI-driven tests.

In this PR, I think I ended up extracting a couple of unit tests as well because of the slow execution time, e.g. testFlatList. If it's feasible to keep them in the unit test target, it would be great. Btw, is there a way to reduce the number of pages used for these tests, assuming they aren't testing performance?

func testFlatList() throws {
         let total = 1000

@crazytonyli

Copy link
Copy Markdown
Contributor

I think that performance tests are not unit tests and hence need to be in a separate target, similar to the UI-driven tests.

Sounds good to me!

Btw, is there a way to reduce the number of pages used for these tests, assuming they aren't testing performance?

It's totally fine to reduce the number of pages. I used a large number just so that it's easier to see the time difference between the old and new implementation.

@kean kean closed this Apr 17, 2024
@kean
kean deleted the task/disable-slow-performance-tests branch April 17, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Unit and UI Tests and Tooling [Type] Task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants