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

test: store nocks in a sqlite database #374

Merged
merged 8 commits into from
Feb 23, 2024

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Feb 12, 2024

Storing the nocks in a SQLite database enables easily sharing nocks between tests run in different processes in parallel.

This reduces the storage space needed for the nocks:

$ du tests/nock
- 186700  tests/nock
+  48968  tests/nock

It avoids the v8.serialize / v8.deserialize issues encountered in #365.

The database is portable so the version of Node.js used to generate it doesn't matter.

@merceyz merceyz changed the title test: store nocks in a sqlite3 database test: store nocks in a sqlite database Feb 12, 2024
@merceyz merceyz marked this pull request as ready for review February 13, 2024 00:00
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this approach will not introduce a conflict-hell, if virtually all PRs will touch the same binary file.

tests/recordRequests.js Outdated Show resolved Hide resolved
tests/recordRequests.js Outdated Show resolved Hide resolved
tests/recordRequests.js Outdated Show resolved Hide resolved
tests/recordRequests.js Outdated Show resolved Hide resolved
@merceyz
Copy link
Member Author

merceyz commented Feb 18, 2024

I wonder if this approach will not introduce a conflict-hell, if virtually all PRs will touch the same binary file.

Yeah, that's the downside of this approach, any new nock will cause a conflict.
However since nocks can be re-used between tests it's possible the nock is already in the database.
If it becomes too much we can always add some automation to handle it.

Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems a good idea; slightly worried about the risk of conflicts too (especially if it breaks the "Update this PR" button in GH), but it's probably fine to try it and iterate as needed.

@merceyz merceyz merged commit 967e266 into nodejs:main Feb 23, 2024
10 checks passed
@merceyz merceyz deleted the merceyz/test/sqlite3 branch February 23, 2024 23:59
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.

None yet

3 participants