The test stream needs to be closed #1864
Conversation
|
Thanks for the PR. Unfortunately, this can't be merged because there are broken tests on master. Suddenly they stopped working and I don't know how to fix them. |
|
I suspect that the tests are failing due to CompressionStream becoming available on android, and / or the implementation changing slightly. I have attempted to upgrade pako, whose issues repo mentions some compatibility issues that have been fixed. I will see how the pipeline tests go. It comes down to the compression method in the utils/deflate.js file in isomorphic-git giving a different hash output on some platforms. I'm not entirely sure it matters, since the decompression algorithm will work fine either way, but it would be nice to have the same outputs on all platforms, just because its neater. We should be prepared to let that constraint go if fixing this is too costly - we do not need binary output equivalence on all platforms to have functional output equivalence. |
|
But the problem is that testing is always on the same platforms and suddenly it stops working. Nothing was updated. All PRs that were merged successfully passed all the tests. |
inverted-capital
force-pushed
the
patch-1
branch
from
8034d6a
to
d906d1c
Compare
|
I think what has happened is that the platforms that are used for the testing have updated something internally that changes how they do compression. I am poking around to try find out why. |
|
Ok so I think that's the problem right there - CompressionStream on the Android platform must have changed recently, and it now produces something binarily different to what it did before. This may be because of how we're using it, but it might be a deeper bug. We can either take the blunt fix I've put in here, or we can sniff if we are on the android platform and deny use of CompressionStream in that case until we know how to handle it better. Do you guys have a way to sniff Android in this codebase already ? |
|
I don't like deleting the code like this, instead of figuring out what exactly is happening with the CompressStream on Android. Also, the problem with deleting this code is that it has to work in the browser. I'm not sure if pako is included with the library for use inside the browser. That code was there for a reason. |
inverted-capital
force-pushed
the
patch-1
branch
from
42ea0e3
to
1c349d3
Compare
|
Ok - I understand your reluctance to remove code like this.
The tests were working 2 weeks ago, then something on android changed and now the tests are not working, even tho nothing changed here. That indicates something inside Android changed, and I don't have the ability to trace that deeper. How about we just force pako only in Android, and then when Android changes back to match every other platform, we remove the conditional check ? If we don't do this, then all PRs will be blocked until Android changes its behaviour back, or someone figures out what's going on there, which could be some time. |
|
Ok so I've done a sniff for Android and the tests pass. The tests are running on Chrome 120 on Android 10 - this version of Chrome was released on Nov 28 - my best guest is there's a subtle discrepancy in there about the binary output of CompressionStream. I cannot feasibly trace deeper than this. |
|
The last PR to pass on Jan8 ran the android tests on chrome 117 |
|
I created an issue in the Chromium project. Let's see what they will say: https://issues.chromium.org/issues/324707366 I hope they will not close the issue as off-topic. I don't know any other way of contacting the developers of the Chromium project. |
|
thank you 🙏 that is a good issue to raise with Chromium. Perhaps you could link them to the exact line of the log to look at, since they might not bother scrolling ? https://dev.azure.com/isomorphic-git/isomorphic-git/_build/results?buildId=3277&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=5b4cc83a-7bb0-5664-5bb1-588f7e4dc05b&l=16537 |
|
They replied that zlib library got updated. I'm not sure what the tests are doing but if they compare binary data output from the compression stream, those checks should be removed since the output can't change. It's not a fixed algorithm that will always output the same data. |
|
I disagree with their opinion. This is the data format for zlib. I believe their implementation is out of spec, but I do not have the resources to say exactly why. I believe maintaining binary compatibility should be upheld for isomorphic-git. We have working binary compatibility on Android provided by this PR, and my advice is to continue to treat Android as a special case until such time as their platform comes into compliance OR the other platforms move in the same way. I am unsure if we have tests that compress on Android, push to Ubuntu, and decompress there. This type of usage might break if binary compatibility is abandoned. The only loss from merging this PR is some performance degradation on Android. |
|
I've sent an email to Zlib authors, let's see what they would say about this. I would really not like to disable native compression just for Android only try to fix the unit tests. |
|
I was able to fix the test and didn't compare any compressed blobs. The fixture is still binary pack file as before and the content of that pack file is read and compared with created pack file. You can remove your check to ignore compressStream for Android and I will merge/squash all the changes. Make sure to rebase with the main branch. |
|
Ok I will do that. I am concerned that with the changes you have made, there may be no testing path between creating a new pack file on Android with the out-of-spec native compression format, and opening that pack file on another platform. Thank you for your persistence in tracing down this issue - much appreciated - this is a great library and very useful to me🙏 |
inverted-capital
force-pushed
the
patch-1
branch
3 times, most recently
from
1e7a789
to
51112d0
Compare
|
There is no way you can test that. |
|
🎉 This PR is included in version 1.25.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Otherwise resources are left dangling