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

WIP: Fix #6736: Update bun to latest version in CI #6737

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

agateblue
Copy link

@agateblue agateblue commented Jan 6, 2025

Cf #6736, Opening this now to trigger a test run in CI and see how it goes, I expect the Dexie suite to fail under bun 1.1.42

Todos

  • Tests

@agateblue agateblue marked this pull request as draft January 6, 2025 17:11
@agateblue
Copy link
Author

It seems tests were already failing in the same manner as described in #6736 under bun 1.1.18, for instance: https://github.com/pubkey/rxdb/actions/runs/12634621221/job/35202766323#step:10:186

Increasing the timeout globally, e.g running with bun run test:bun:dexie -- --timeout 50000 gives me hundreds of green tests, but many of them are running extremely slow. The find all ones takes more than 28 seconds! See below for some examples:

  rx-collection.test.ts
    static
      .addCollections()
        ✔ should not crash (3053ms)
      .create()
        ✔ human (968ms)
        ✔ should not forget the options (967ms)
        ✔ if not premium, it should limit the collections amount (1432ms)
      .checkCollectionName()
        ✔ allow not allow lodash
        ✔ allow numbers
        ✔ not allow starting numbers
        ✔ not allow uppercase-letters
    instance
      .insert()
        ✔ should insert a human (211ms)
        ✔ should insert nested human (210ms)
        ✔ should insert more than once (264ms)
        ✔ should set default values (214ms)
        ✔ should throw a conflict-error (214ms)
        ✔ should not allow wrong primaryKeys (263ms)
        ✔ should throw a helpful error on non-plain-json data (219ms)
        ✔ should throw a helpful error message on Date() objects (218ms)
      .bulkInsert()
        ✔ should insert some humans (6987ms)
        ✔ should not throw when ids are set in pre-insert hook (6986ms)
        ✔ should not throw when called with an empty array (78ms)
        ✔ should throw if one already exists (6989ms)
        ✔ should throw on duplicate ids (77ms)
      .find()
        find all
          positive
            ✔ find all (28286ms)
            ✔ find 2 times (28017ms)
            ✔ find in serial #0 (1720ms)
            ✔ find in serial #1 (3809ms)
            ✔ find all by empty object (27436ms)
            ✔ find nothing with empty collection (1684ms)
            ✔ BUG: insert and find very often (0) (4015ms)
            ✔ BUG: insert and find very often (1) (4990ms)
          negative
            ✔ should crash with string as query (25810ms)
            ✔ should crash with array as query (27176ms)
        $eq
          ✔ find first by passportId (21015ms)
          ✔ find none with random passportId (20804ms)
          ✔ find via $eq (21013ms)
        .or()
          ✔ should find the 2 documents with the or-method (10020ms)
          ✔ should find the correct documents via $or on the primary key (10023ms)
        .sort()
          ✔ sort by age desc (with own index-search) (1245ms)
          ✔ sort by age desc (with default index-search) (1243ms)
          ✔ sort by age asc (1243ms)
          ✔ sort by non-top-level-key as index (with keycompression) (1036ms)
          ✔ validate results (1041ms)
          ✔ find the same twice (899ms)
          ✔ sort by compound index with id (1241ms)
          ✔ #146 throw when field not in schema (object) (1034ms)
          ✔ #146 throw when field not in schema (string) (1033ms)
        .limit()
          ✔ get first (8843ms)
          ✔ get last in order (8825ms)
          ✔ reset limit with .limit(null) (8841ms)
        .skip()
          ✔ skip first (181ms)
          ✔ skip first in order (235ms)
          ✔ skip first and limit (storage: dexie) (230ms)
          ✔ skip first and limit (storage: dexie) (229ms)
          ✔ reset skip with .skip(null) (2814ms)
        .regex()
          ✔ find the one where the regex matches (8738ms)
          ✔ case sensitive regex (8736ms)
          ✔ regex on index (8736ms)
          ✔ should not allow to use RegExp objects (8658ms)
        .remove()
          ✔ should remove one document (920ms)
          ✔ should remove all documents (1063ms)
          ✔ should remove only found documents (1060ms)
          ✔ remove on findOne (1050ms)
          ✔ #3785 should work when the collection name contains a dash or other special characters (1050ms)
          ✔ #3788 removing the collection should also remove all changes (1014ms)
          ✔ #5721 should remove the RxCollection instance across tabs and emit the .$removed event (1045ms)
        .bulkRemove()
          ✔ should remove some humans (6454ms)
          ✔ should not throw when called with an empty array (60ms)
        .update()
          ✔ sets a field in all documents (216ms)
          ✔ unsets fields in all documents (389ms)
      .findOne()
        ✔ find a single document (2991ms)
        ✔ not crash on empty db (102ms)
        ✔ find different on .skip() (3246ms)
        ✔ find by primary (2990ms)
        ✔ find by primary in parallel (102ms)
        ✔ BUG: insert and find very often (0) (102ms)
        ✔ BUG: insert and find very often (1) (102ms)
        ✔ BUG: should throw when no-string given (number) (2731ms)
        ✔ BUG: should throw when no-string given (array) (2731ms)
      .count()
        ✔ should count one document (131ms)
        ✔ should not count deleted documents (200ms)
        ✔ count matching only (131ms)
        ✔ must throw when query has property of selectorSatisfiedByIndex=false (89ms)
        ✔ must throw on limit and skip (89ms)
        ✔ #4775 count() broken on primary key (129ms)
      .bulkUpsert()
        ✔ insert and update (3477ms)
        ✔ issue: insert and update+insert breaks (686ms)
        ✔ should throw on duplicate ids (663ms)
      .upsert()
        ✔ insert when not exists (147ms)
        ✔ overwrite existing document (160ms)
        ✔ overwrite twice (163ms)
        ✔ overwrite deleted (165ms)
        ✔ throw when primary missing (143ms)
      .incrementalUpsert()
        positive
          ✔ should work in serial (987ms)
          ✔ should not crash when upserting the same doc in parallel (986ms)
          ✔ should not crash when upserting the same doc in parallel 3 times (1001ms)
          ✔ should not crash when upserting the same doc in parallel many times with random waits (1078ms)
          ✔ should update the value (1013ms)
          ✔ should work when upserting to existing document (1001ms)
          ✔ should process in the given order (999ms)
          ✔ should work when inserting on a slow storage
          ✔ should set correct default values (982ms)
          ✔ should completely remove fields that are unset (982ms)
      .remove()
        ✔ should not crash (212ms)
        ✔ should be possible to re-create the collection with different schema (221ms)
        ✔ should not contain document when re-creating (266ms)
        ✔ should have deleted the local documents (221ms)
        ✔ should delete when older versions exist (368ms)
        ✔ should not have the collection in the collections-list (207ms)
      .findByIds()
        ✔ should not crash (2150ms)
        ✔ should find the documents (2103ms)
        ✔ should find the documents when they are not in the docCache (2109ms)
        ✔ we should be able to modify the documents (2125ms)
        ✔ we should be able to patch the documents (2124ms)
        ✔ #6148 using chained queries on a find-by-id-query should throw a proper error message (2148ms)
    .findByIds.$()
      ✔ should not crash and emit a map (2974ms)
      ✔ should emit the correct initial values (2973ms)
      ✔ should merge the insert/update/delete event correctly (3002ms)
    issues
      ✔ #528  default value ignored when 0 (2973ms)
      ✔ #596 Default value not applied when value is undefined (1993ms)
      ✔ #939 creating a collection mutates the given parameters-object (3016ms)
      ✔ #3661 .findByIds.$() fires too often (4902ms)

@pubkey, given the pre-existing timeouts in test runs under 1.1.18, I suspect the slowness and associated failures under this runtime have been here for quite some time. In fact, I cannot seem to find a github workflow where the test suite passed. It was already failing two months ago, like here: https://github.com/pubkey/rxdb/actions/runs/11912718263/job/33196921290#step:10:197

Even with a 50s timeout, I'm still finding tests that fail because they are too slow, thus I'm not sure what the best course of action is.

Maybe set a 30s timeout and simply skip the tests that fail above that limit, to still have some coverage? What do you think?

@agateblue agateblue force-pushed the bun branch 2 times, most recently from 8a1f81c to 347cc5f Compare January 6, 2025 18:15
@pubkey
Copy link
Owner

pubkey commented Jan 7, 2025

Still times out. Please run it locally first and ensure it works on your device.

@agateblue
Copy link
Author

agateblue commented Jan 7, 2025

With the last commits and a 60s timeout, it runs on my device:

  database-lifecycle.ts
    ✔ do some writes updates and deletes and cleanups and reopens (8803ms)

  last.test.ts (dexie)
    ✔ ensure all Memory RxStorage instances are closed
    ✔ ensure every db is cleaned up
    ✔ ensure all collections are closed
    ✔ ensure all BroadcastChannels are closed
    ✔ ensure all RemoteMessageChannels have been closed
    ✔ ensure all websockets have been closed
    ✔ exit the process


  974 passing (7m)

I had to disable a dozen slow tests, fix an actual comparison issue, disable a test crashing for an unknown reason, and disable compressionstream related test (the API isn't implemented in bun).

In comparison to the standard node runtime test suite, bun takes 3-4x more times, and misses ~140 tests:

  # from: https://github.com/pubkey/rxdb/actions/runs/12647741628/job/35240707472#step:7:1
  database-lifecycle.ts
    ✔ do some writes updates and deletes and cleanups and reopens (1039ms)

  plugin.test.js
    .addRxPlugin()
      ✔ should not crash when a new plugin is added
      ✔ should crash when a plugin with the same name added already but it is NOT the same object
      ✔ should NOT crash when a plugin with the same name added already but it IS the same object
    full.node.ts
      ✔ full.node.ts should run without errors (3511ms)
    hooks
      ✔ createRxDatabase
      ✔ createRxCollection
      ✔ createRxSchema
      ✔ createRxDocument
      ✔ postCreateRxDocument (211ms)
      ✔ postCleanup (210ms)

  last.test.ts (dexie)
    ✔ ensure all Memory RxStorage instances are closed
    ✔ ensure every db is cleaned up
    ✔ ensure all collections are closed
    ✔ ensure all BroadcastChannels are closed
    ✔ ensure all RemoteMessageChannels have been closed
    ✔ ensure all websockets have been closed
    ✔ exit the process

  1102 passing (2m)

Some steps/tests are an under of magnitude slower under bun, at least on my computer:

  • Node: do some writes updates and deletes and cleanups and reopens (1039ms)
  • Bun: do some writes updates and deletes and cleanups and reopens (8803ms)

I'm waiting for a CI run to see if it passes (or at least runs more tests than last time before crashing). As far as I can tell, the slowness under bun is I/O bound, because the CPU were mostly idle during test runs. And by the way I'm not saying we should merge this in its current state, but a (mostly) working test suite is an improvement given the prior situation and if the CI confirms my local runs, maybe we can decide how to procede next?

@pubkey
Copy link
Owner

pubkey commented Jan 7, 2025

These tests run on the in-memory indexeddb. They should neither be slow nor commented out. Lets check the CI.

@agateblue
Copy link
Author

So, the CI run took 10m, but all the 974 tests passed. How would you like to proceed?

@pubkey
Copy link
Owner

pubkey commented Jan 8, 2025

We need to fix these performance problems. The tests run on in-memory storage only. There is something going wrong. Also having a 10min run for this should not be the case. The in-memory tests in node.js in fast mode take about 20 seconds.

@agateblue
Copy link
Author

There is something going wrong

Agreed.

I've tried several profiling tools to try to pinpoint the culprit:

If you have any pointers to profile performance under bun/motcha, I can dedicate a few more hours to this. Otherwise, I think I'm going to stop the task there as it's way beyond my skillset and what I assumed to be the original task ("Update Bun.js to the latest version while ensuring the tests still work"). The bun test suite didn't work in the first place, even under bun 1.1.18. It's been failing for at least two months, and I suspect it was the case even before that.

@pubkey
Copy link
Owner

pubkey commented Jan 8, 2025

Maybe we should switch from using dexie+in-memory-indexeddb to just use the memory RxStorage for the bun tests. This would make it easier to find out why it is slow and where exactly it is slow.

@agateblue
Copy link
Author

Maybe I did it wrong, but with the memory storage (see the last commit), tests run in 4 minutes on my machine (versus 7 before), and yields 5 new failure. It's not blazing fast, but it's still faster:

    ✔ ensure all RemoteMessageChannels have been closed
    ✔ ensure all websockets have been closed
    ✔ exit the process


  966 passing (4m)
  5 failing

  1) rx-schema.test.ts
       #4951 patternProperties are allowed:

      AssertionError [ERR_ASSERTION]: myDocument.get('tags').hello
      + expected - actual


      at /home/agate/projects/rxdb/test/unit/rx-schema.test.ts:1166:19
      at processTicksAndRejections (native)

  2) last.test.ts (memory)
       ensure all Memory RxStorage instances are closed:
     Error: not all memory instances have been closed (2 still open)
      at /home/agate/projects/rxdb/test/unit/last.test.ts:40:13
      at processTicksAndRejections (native)

  3) last.test.ts (memory)
       ensure every db is cleaned up:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

      + expected - actual

      -1
      +0
      
      at /home/agate/projects/rxdb/test/unit/last.test.ts:44:15

  4) last.test.ts (memory)
       ensure all collections are closed:
     Error: not all collections have been closed (1)
      at /home/agate/projects/rxdb/test/unit/last.test.ts:55:13
      at processTicksAndRejections (native)

  5) last.test.ts (memory)
       ensure all BroadcastChannels are closed:
     Error: not all broadcast channels have been closed (1)
      at /home/agate/projects/rxdb/test/unit/last.test.ts:67:13
      at processTicksAndRejections (native)



error: script "test:bun:dexie" exited with code 5

@pubkey
Copy link
Owner

pubkey commented Jan 16, 2025

Hey, could you fix these issues? Are you still working on this?

@agateblue
Copy link
Author

The test suite was passing under dexie+memory, switching to memory storage as you recommended didn't really speed up the CI, but did break new tests for unknown reasons. I don't think it makes sense to fix these until we understand why the performance is so bad under bun.

You said that:

Maybe we should switch from using dexie+in-memory-indexeddb to just use the memory RxStorage for the bun tests. This would make it easier to find out why it is slow and where exactly it is slow.

How does it make it easier to pinpoint the root cause? I am unable to debug the performance issues without additional pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants