MatrixAI / js-encryptedfs

Encrypted Filesystem for TypeScript/JavaScript Applications
https://polykey.com
Apache License 2.0
10 stars 3 forks source link

ci: merge staging to master #68

Closed MatrixAI-Bot closed 1 year ago

MatrixAI-Bot commented 1 year ago

This is an automatic PR generated by the pipeline CI/CD. This will be automatically fast-forward merged if successful.

Fixes #69

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 595698870 for 158c0c7e749293b7a6a7a698d7f88312109e2633

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/595698870

CMCDragonkai commented 1 year ago

There are test failures on windows and macos. Have a look into why that is the case using matrix-win-1 and matrix-mac-1.

However I was thinking, let's just update js-logger here. When you inject it into the DB which hasn't been updated, use // ts-ignore. Because runtime-wise it should work the same, it's just a type incompatibility. That way we can retarget #67 to only focus on js-db upgrade which is low priority.

emmacasolin commented 1 year ago

There are test failures on windows and macos. Have a look into why that is the case using matrix-win-1 and matrix-mac-1.

However I was thinking, let's just update js-logger here. When you inject it into the DB which hasn't been updated, use // ts-ignore. Because runtime-wise it should work the same, it's just a type incompatibility. That way we can retarget #67 to only focus on js-db upgrade which is low priority.

Could there potentially be issues with an injected logger coming from PK that wants the formatting to be json? Because if this is passed through to EFS's DB then the logger there wouldn't have that functionality. Would it cause a runtime error or just be ignored?

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 596595197 for 442a61f4ca9a75e32283440e1524e62ca219acca

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/596595197

CMCDragonkai commented 1 year ago

There are test failures on windows and macos. Have a look into why that is the case using matrix-win-1 and matrix-mac-1.

However I was thinking, let's just update js-logger here. When you inject it into the DB which hasn't been updated, use // ts-ignore. Because runtime-wise it should work the same, it's just a type incompatibility. That way we can retarget #67 to only focus on js-db upgrade which is low priority.

Could there potentially be issues with an injected logger coming from PK that wants the formatting to be json? Because if this is passed through to EFS's DB then the logger there wouldn't have that functionality. Would it cause a runtime error or just be ignored?

No the bugs on windows and Mac are separate issues. Have a look at the logs and try cloning and reproducing it on the 2 machines.

CMCDragonkai commented 1 year ago

So runtime wise, when you switch to JSON formatting this only occurs for the root logger so it won't affect the child loggers.

emmacasolin commented 1 year ago

Having a look at the failures on windows now. The mac failure on the cicd looks to be just a timeout, however, matrix-mac-1 is really slow right now (I can't even checkout the staging branch) so I can't test if it's timing out there as well.

emmacasolin commented 1 year ago

There are a LOT of failures on Windows, all of them in the EncryptedFS.* tests (concurrency, dirs, files, links, and EncryptedFS.test.ts). A lot of these errors also look similar to the Windows failures in PK, many of which are in the vaults domain, so many of them likely stem from problems here in the EFS rather than in PK.

CMCDragonkai commented 1 year ago

Alot of concurrency tests may not be robust against timing and scheduling changes. Which is why I wanted to get the better concurrency fast-check utility.

See what you can fix right now, and report new issues for these bugs for the ones that will take more time.

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 598882549 for 6dce66aabbc330fd9b998f03e20aa6989d3395e3

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/598882549

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 598883795 for 861470afb0cc730f06949a198896ade236fbbdb1

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/598883795

CMCDragonkai commented 1 year ago

Failed timeout test on macos:

  ● EncryptedFS Files › write › can make 100 files
    thrown: "Exceeded timeout of 10000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
      384 |       await efs.close(fd);
      385 |     });
    > 386 |     test('can make 100 files', async () => {
          |     ^
      387 |       let content = '';
      388 |       for (let i = 0; i < 50; i++) {
      389 |         const name = 'secret';
      at tests/EncryptedFS.files.test.ts:386:5
      at tests/EncryptedFS.files.test.ts:333:3
      at Object.<anonymous> (tests/EncryptedFS.files.test.ts:13:1)

Seems maybe the macos is just slow.

Anyway the js-db v5 will be alot faster.

For comparison:

windows:
      √ can make 100 files (5843 ms)
linux:
      ✓ can make 100 files (3199 ms)
CMCDragonkai commented 1 year ago

Funnily enough the test only tests 50 files. I'm going to try to see if 25 files will make it pass. Speed improvements will be important for js-db 5.0.0 to compare.

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 598896453 for 07a3c5ae6ade6b1a833c1d2edd5cd0a8e7a2199e

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/598896453

CMCDragonkai commented 1 year ago

Either the macos runner is crazy slow, or the db is just really slow for macos:

      ✓ can make 25 files (7137 ms)

So this will pass now.

CMCDragonkai commented 1 year ago

The next test failure is inconsistent concurrency test. In fact I think it is wrong. @tegefaulkes.

EncryptedFS.read and EncryptedFS.createWriteStream

Says at the end that it read operation will either read all 50 bytes, or it will read 0 bytes.

Why is this so? It could read 5 bytes, 10 bytes, or any multiple in between. The atomicity is per write. It's not between the 2 operations.

tegefaulkes commented 1 year ago

If that's the case then it might just be an oversight of the test. Although wouldn't a write stream 'flush' the writes in blocks?

Just putting the error log here for reference.

``` ● EncryptedFS Concurrency › concurrent file writes › EncryptedFS.read and EncryptedFS.createWriteStream expect(received).toStrictEqual(expected) // deep equality - Expected - 1 + Received + 1 @@ -1,9 +1,9 @@ Array [ Object { "status": "fulfilled", - "value": 0, + "value": 5, }, Object { "status": "fulfilled", "value": undefined, }, 2088 | expect(contents).toEqual(dataA.repeat(10) + '\0'.repeat(50)); 2089 | } else { > 2090 | expect(results).toStrictEqual([ | ^ 2091 | { status: 'fulfilled', value: 0 }, 2092 | { status: 'fulfilled', value: undefined }, 2093 | ]); at Object. (tests/EncryptedFS.concurrent.test.ts:2090:25) ```
CMCDragonkai commented 1 year ago

To fix this, I've integrated the https://github.com/jest-community/jest-extended library.

This package brings in quite a few useful matchers, that we should integrate into PK as well, as it provides some standard matchers that we have been missing for quite a bit.

In particular I'm interested in asymmetric matching and the toSatisfy matcher.

200c0f1 brings in jest-extended, @emmacasolin this can go into staging of PK.

Subsequently for the test itself, I've changed it to be predicate based. This is one step towards "model based testing", where we create a model of the predicates, and then allow simulations to fit the predicates. So subsequently we can bring in fast-check in order to fiddle with the async/promise initial conditions and to ensure that the predicates are always true.

The predicate simply now checks that the bytes read is always a multiple of 5 starting at 0.

I've removed the second variant of delaying by 100ms, this should be done when fast-check is used for general concurrency testing.

CMCDragonkai commented 1 year ago

There could be other tests that are non-deterministic too due to incorrect assumption on promise execution order. Those will need to be fixed with a combination of fast-check and jest-extended in #67.

MatrixAI-Bot commented 1 year ago

Pipeline Attempt on 598964074 for ba683e0b28475c6e2738a8745f379cf8c4e91737

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/598964074

MatrixAI-Bot commented 1 year ago

Pipeline Succeeded on 598964074 for ba683e0b28475c6e2738a8745f379cf8c4e91737

https://gitlab.com/MatrixAI/open-source/js-encryptedfs/-/pipelines/598964074