-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
add asset tests #9802
add asset tests #9802
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4a7e33b:
|
0051a84
to
c65c4d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good! letf one comment about the format containing query params. Can we look into that please?
Other than that PR is great!
src/assets/resolver/Resolver.ts
Outdated
// note: if the key is "format" and there are query params, | ||
// then "format" will contain the query params as well, | ||
// so we check inclusion rather than strict equality | ||
return typeof priorityValue === 'string' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to fix the format? It should ideally be stripped of query params before it gets to here? cc @Zyie does that sound about right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah format shouldn't have a query param on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok wasn't sure if that was intended behavior so didn't change current state. Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, moved extension parsing to small util function, tested that. Reverted bug workaround.
π€ Generated by Copilot at 132ea06
Summary
ππβ
This pull request adds and updates unit tests for the
Assets
,Cache
, andLoader
classes, and fixes a bug in theResolver
class. It also refactors some of the test files and utility functions to improve readability and avoid duplication. The main files affected aretests/assets/*.tests.ts
,tests/utils/async.ts
,src/assets/resolver/Resolver.ts
, andtests/app/Application.tests.ts
.Walkthrough
Resolver
class for assets with query parameters (link)tests/utils/async.ts
and update import paths (link, link, link)basePath
constant totests/assets/basePath.ts
and update import paths (link, link)Assets
class related to asset bundles, video assets, detection parsers, and unresolved assets (link, link, link, link)Cache
class related to custom parsers (link)Loader
class related to loading and unloading different types of assets (link)loadWebFont
function related to loading and unloading web fonts (link)createStringVariations
function related to generating string variations from template strings (link)tests/assets/sampleManifest.ts
for testing asset bundles (link)