Skip to content

(#8581) - Fix this of changesHandler#8583

Merged
janl merged 6 commits intoapache:masterfrom
vrtmrz:8581-fix-this
Feb 2, 2023
Merged

(#8581) - Fix this of changesHandler#8583
janl merged 6 commits intoapache:masterfrom
vrtmrz:8581-fix-this

Conversation

@vrtmrz
Copy link
Copy Markdown
Contributor

@vrtmrz vrtmrz commented Dec 29, 2022

Fix this of changesHandler.

The test is also written but works only for browsers.

@janl
Copy link
Copy Markdown
Member

janl commented Feb 1, 2023

@vrtmrz thank you for this. Unfortunately the test fails in CI: https://github.com/pouchdb/pouchdb/actions/runs/4063298839/jobs/6995899992 — I’m not sure I understand the error message about number of assertions either. Is there something you could do?

- Rewrite the function to callback-style.
- Fix for timings.
@vrtmrz
Copy link
Copy Markdown
Contributor Author

vrtmrz commented Feb 2, 2023

Thank you for pointing it out!
I am not sure about the value that assertion detected. However, we may also have timing problems.
I have rewritten the function to a callback style and will test it again.

Fix format issues.
- Reorder for confirming `equal(7)`
@vrtmrz
Copy link
Copy Markdown
Contributor Author

vrtmrz commented Feb 2, 2023

I have checked this test again. The assertion error you pointed out was probably not a problem with this test itself.
Perhaps, test.viewadapter.js is not waiting for both of onsuccess, and this test detects it.
I have not tested these assumptions, but I think we need to address and call done() or wrap events to Promise and await.

The browser-side tests became can be passed. May I ask you to review it again?

@janl janl merged commit 06bfe89 into apache:master Feb 2, 2023
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.

3 participants