-
Notifications
You must be signed in to change notification settings - Fork 0
Tests/add kubo if not #61
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
Conversation
…kip integration tests
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Before continuing further on this would like to hear your guys' thoughts if it makes sense as a path forward |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## refactor #61 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 5 5
Lines 597 597
==========================================
Hits 597 597 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
abidsikder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fixture launched a fresh ipfs node so that there were no other effects from ipfs nodes having prior CIDs or other data when testing performance. But for correctness issues having one ipfs node running for all tests should not impact things.
We were doing an ipfs init on with every fixture since it was set at module vs session. This meant that tests took ~45 seconds to run because ipfs would have to be initialized for every function. When swapped to session it's made once for the test which makes the tests go much faster (since CIDs are unique this is ok?). I do want to hear what @abidsikder thinks about this though when making the tests if there was a reason it was chosen that ipfs would start fresh on every test.
Beyond that it was also an issue to have to always have your ipfs daemon running to run tests. Normally it's not a problem, and the github action spawns an ipfs node as well, but what if the end user doesn't have an IPFS daemon running. In theory they shouldn't have to. If they had docker installed that should suffice. This was also precipitated by trying to get this library to run the tests within an isolated AI context but tests failed because it did not have IPFS running and all I could think was...hmm shouldn't that be contained already within the library itself (aka spawning the ipfs node?)
More tests but runs in about half the time due to the shared session.