Repository navigation
Isolated Processes 19: Integrate remote backend - #219
Conversation
d36199c to
2ddadbc
Compare
f6d6bff to
b82c28d
Compare
2ddadbc to
16c1eff
Compare
b82c28d to
42b9b3a
Compare
16c1eff to
cc0216b
Compare
42b9b3a to
98012cf
Compare
cc0216b to
bd97207
Compare
ec39115 to
acce18a
Compare
bd97207 to
091d71f
Compare
acce18a to
f5a0634
Compare
091d71f to
f432734
Compare
|
@gabrielesvelto I made the changes to the testing that I said I would. I think you'll agree this is a lot better. Should be good to review now! |
There was a problem hiding this comment.
LGTM overall but there's something I may be missing: having replaced most configurations with those generated via remote_mw_config() aren't we only testing the remote path now? Shouldn't we run the tests both with and without a remote executor?
| ) -> Result<Vec<u8>, CopyFromProcessError> { | ||
| let length = | ||
| std::num::NonZeroUsize::new(length).ok_or(CopyFromProcessError::InvalidArgument)?; | ||
| let mem = process_inspector.process_reader(); | ||
| mem.read_to_vec(src, length) | ||
| mem.read_all_to_vec(src, length) |
There was a problem hiding this comment.
I think this may have spilled over from another commit (part 9 maybe?), anyway no big deal.
There was a problem hiding this comment.
You'd think, but no -- As it turns out, this is actually a bug! There are several pieces of code that call copy_from_process() that basically don't work if it only returns a partial read -- It seems that the assumed contract for copy_for_process() is that it's all-or-nothing -- It's just that partial reads never practically occurred until IO buffering entered the fold.
We could, but TBH I was a bit on the fence about it -- Simply because the remote path necessarily tests the local path as part of the entire chain. The executor uses the local backend within its own process to execute the remote requests. So I don't know... I get that there's value in testing the different combinations of things sometimes, but there's also value in not having a unit testing suite that takes a long time to execute because it tests everything twice... |
That's true. In fact, once the integration is done, I was considering adding some tests in Firefox automation that would test the remote path on regular Linux rather than Android, to exercise this code even before the mobile team merges the isolated process support. |
That seems like a good idea! There probably should be some end-to-end testing for this. I think it would just be a pref anyway to run the crash reporter with the remote executor. It definitely works, because I do most of the testing on desktop Linux before I test on Android. |
No description provided.