Add iOS Preload Tests #253
Conversation
jkmassel
force-pushed
the
add/preload-list-tests
branch
3 times, most recently
from
8876355 to
bee4474
Compare
jkmassel
force-pushed
the
add/preload-list
branch
from
71f68fa to
c8d4f11
Compare
jkmassel
force-pushed
the
add/preload-list-tests
branch
2 times, most recently
from
b466e2a to
99ce8f9
Compare
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Thank you for the thorough test coverage!
I skimmed the test names, and a few dozen test implementations. The tests look good to me.
Some of the more granular tests seem less valuable (e.g., Foundation extensions). Hopefully their long-term value exceeds any maintenance cost—presumably we won't change these much and generative AI should make changes easier. I'm not opposed to keeping them, but we could consider reduction or consolidation if deemed worthwhile.
ios/Tests/GutenbergKitTests/Resources/editor-asset-manifest-test-case-1.json
Show resolved
Hide resolved
jkmassel
force-pushed
the
add/preload-list-tests
branch
2 times, most recently
from
543e9e1 to
64284af
Compare
dcalhoun
force-pushed
the
add/preload-list-tests
branch
from
209696f to
f59acb1
Compare
|
I rebased atop the latest trunk branch with the following command to make review easier: git rebase --onto origin/trunk origin/add/preload-list add/preload-list-tests |
|
|
||
| # Build Swift package (force refresh of npm deps/translations if needed) | ||
| make build-swift-package REFRESH_DEPS=1 REFRESH_L10N=1 | ||
| swift build |
There was a problem hiding this comment.
Is the usage of swift over make to avoid required simulators or some other purpose?
There was a problem hiding this comment.
The make stuff is used in CI, which builds the JS. Claude kept running it unnecessarily which was slow, and also polluted my git repo with the JS changes
There was a problem hiding this comment.
@jkmassel makes sense.
I've been addressing similar inefficiencies elsewhere by introducing REFRESH_[target] environment variables for the npm-dependencies and prep-translations targets. We might consider adding a similar REFRESH_JS_BUILD here.
My goal has been to structure the Makefile to be the de facto interface for the project, where all documentation for users and LLMs point to the same scripts. Where the Makefile...
- Always ensures the correct dependencies are in place for a given target;
- Delivers efficient re-runs (for local runs);
- And provides necessary environment variables for full control when necessary (for CI servers or resets).
Not stating we must revert these changes, just offering an alternative for consideration.
There was a problem hiding this comment.
I'm not 100% sure I understand, but I agree with it in principle so I'd be happy to 👍 a PR that does this?
jkmassel
force-pushed
the
add/preload-list-tests
branch
from
f59acb1 to
c82027d
Compare
|
@jkmassel it seems the automated Swift tests fail in trunk after this merge. Strangely, the tests seemingly pass for PR runs. |
Ideally, the `makefile` serves as the de facto interface for project scripts. This avoids multiple sources of truth and simplifies project onboarding and documentation. Previously, rerunning various `make` targets resulted in rerunning the expensive and slow `make build` target. It's important to ensure the build output exists for many targets, but is often unnecessary to rerun the build when running targets multiple times--e.g., Swift test targets. This introduces a caching mechanism. The `make build` target skips running and relies upon the cache unless one of the following is true. - dist doesn't exist - REFRESH_JS_BUILD is set to true or 1 - build was invoked directly This enables quicker reruns while also ensure the build is always recreated when important to do so--e.g., direct invocations, CI runs, releases. See #253 (comment)
Just the tests for #250, split out because there's a lot of code.