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

Resolve some test issues #71

Merged
merged 2 commits into from
Mar 18, 2023

Conversation

lorentey
Copy link
Member

@lorentey lorentey commented Mar 18, 2023

  1. Disable test registration outside of CMake builds.

    This will allow this code base to be built with build systems other than CMake and SPM without these test manifests suddenly appearing again.

  2. Split Basics.swift.gyb into 26 different files, one for each test case.

    The original autogenerated swift file is so humongous that it causes problems for some folks.

  3. Fix CMake configuration for package tests.

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (if appropriate).
  • I've verified that my change does not break any existing tests.
  • I've updated the documentation if necessary.

@lorentey lorentey added this to the 1.1.0 milestone Mar 18, 2023
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey lorentey requested a review from glessard March 18, 2023 06:45
@lorentey lorentey force-pushed the tie-test-registration-to-cmake branch from 90cc63c to 12cc95b Compare March 18, 2023 07:29
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey lorentey force-pushed the tie-test-registration-to-cmake branch from 12cc95b to 45772d9 Compare March 18, 2023 07:49
@lorentey
Copy link
Member Author

@swift-ci test

The original autogenerated swift file is so humongous that it causes problems for some folks.

Split it up into one file per XCTestCase.
@lorentey lorentey changed the title Disable test registration outside of CMake builds Resolve some test issues Mar 18, 2023
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey
Copy link
Member Author

lorentey commented Mar 18, 2023

Wow. Splitting up that huge test file reduced CI build times from around 10 minutes to roughly 1 minute, on both macOS and Linux. 🤯
Screenshot 2023-03-18 at 1 37 37 AM

@lorentey
Copy link
Member Author

@swift-ci test

@lorentey
Copy link
Member Author

Compiling this one file probably needed too much memory for the CI machines, and so they started thrashing, leading to 10x slower build times than expected.

This also explains the off hand comments I sometimes saw about not being able to build tests at all on constrained machines -- sadly I can't recall any actual bug reports about that, so it remained undiagnosed.

@lorentey lorentey merged commit 40569d6 into apple:main Mar 18, 2023
@lorentey lorentey deleted the tie-test-registration-to-cmake branch March 18, 2023 20:02
@tayloraswift
Copy link
Contributor

Wow. Splitting up that huge test file reduced CI build times from around 10 minutes to roughly 1 minute, on both macOS and Linux. exploding_head

this whole time i thought the problem was vague type inference… i did not know source file length could have such an impact on typechecker performance…

@lorentey
Copy link
Member Author

lorentey commented Mar 18, 2023

I suspect it's all about the sheer amount of memory that the compiler needed to hold the whole thing all at once. This also explains why I never really saw this in my local builds -- I don't typically try building things on memory-constrained systems.

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