Skip to content
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

ci: merge staging to master #68

Merged
merged 10 commits into from
Jul 28, 2022
Merged

ci: merge staging to master #68

merged 10 commits into from
Jul 28, 2022

Conversation

MatrixAI-Bot
Copy link
Member

@MatrixAI-Bot MatrixAI-Bot commented Jul 25, 2022

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

Fixes #69

- Continuous benchmarking (CI/CD)
- Platform tests (CI/CD)
- Linting/general CI/CD updates
@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot MatrixAI-Bot self-assigned this Jul 25, 2022
@CMCDragonkai
Copy link
Member

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.

tsconfig.json Outdated Show resolved Hide resolved
@emmacasolin
Copy link
Contributor

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
Copy link
Member Author

@CMCDragonkai
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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.

Using `path.join` caused the tests to use the windows' separator for the path which is not supported by the EFS. This lead to a lot of test failures.

#69
@MatrixAI-Bot
Copy link
Member Author

…oin` to disambiguate with `utils.pathJoin`
@MatrixAI-Bot
Copy link
Member Author

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jul 28, 2022

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
Copy link
Member

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
Copy link
Member Author

@CMCDragonkai
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

tegefaulkes commented Jul 28, 2022

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.<anonymous> (tests/EncryptedFS.concurrent.test.ts:2090:25)

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jul 28, 2022

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
Copy link
Member

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
Copy link
Member Author

@MatrixAI-Bot
Copy link
Member Author

@MatrixAI-Bot MatrixAI-Bot merged commit ba683e0 into master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

EncryptedFS.rmdir does not remove files on Windows
4 participants