Combine InMemoryAssetReader and InMemoryAssetWriter into InMemoryAssetReaderWriter
#3854
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
davidmorgan
force-pushed
the
unify-write-read
branch
4 times, most recently
from
b85997a to
9fb55d1
Compare
…yAssetReaderWriter`. Update tests so that reader and writer are backed by the same `InMemoryAssetReaderWriter`.
davidmorgan
force-pushed
the
unify-write-read
branch
from
9fb55d1 to
6a66ed9
Compare
| @@ -311,38 +338,46 @@ void main() { | |||
| rootPackage('a', path: path.absolute('a')): ['b'], | |||
| package('b', path: path.absolute('a', 'b')): [] | |||
| }); | |||
| readerWriter = InMemoryRunnerAssetReaderWriter( | |||
There was a problem hiding this comment.
Why does this (and another on line 472) create a new one (which doesn't have the package config written)? If this is what we want a comment explaining why would be good.
There was a problem hiding this comment.
Thanks, good point: the root package is always a so there is no need to create a new one, removed in both places.
_test_common/lib/test_phases.dart
Outdated
| @@ -70,12 +68,11 @@ void _printOnFailure(LogRecord record) { | |||
| Future<BuildResult> testBuilders( | |||
| List<BuilderApplication> builders, | |||
| Map<String, /*String|List<int>*/ Object> inputs, { | |||
| BuildResult? resumeFrom, | |||
There was a problem hiding this comment.
Why do it this way? The whole Expando thing seems like somewhat of a hack, and unless there's a good reason I'd prefer not to use it.
There was a problem hiding this comment.
Thanks, done.
I was trying to avoid changing the return type to avoid such a big breaking change to tests ... but I was confused, I thought this was in build_test which is published. But fortunately it's in _test_common which we can just change. So, changed the return type :)
_test_common/lib/test_phases.dart
Outdated
| @@ -70,12 +68,11 @@ void _printOnFailure(LogRecord record) { | |||
| Future<BuildResult> testBuilders( | |||
| List<BuilderApplication> builders, | |||
| Map<String, /*String|List<int>*/ Object> inputs, { | |||
| BuildResult? resumeFrom, | |||
There was a problem hiding this comment.
Thanks, done.
I was trying to avoid changing the return type to avoid such a big breaking change to tests ... but I was confused, I thought this was in build_test which is published. But fortunately it's in _test_common which we can just change. So, changed the return type :)
For #3811
For my next PR the plan was to give access to the filesystem backing a
Reader.This turned out to be hard to do because of how the tests are set up: the reader and writer are often entirely disconnected. So there is no backing filesystem, even an in-memory one.
This PR refactors to fix that in a lot of cases: so reads and writes in tests are backed by one
InMemoryAssetReaderWriterinstance.