-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Bee class functionality with error handling #15
Conversation
b17bfbb
to
82f7b5f
Compare
test/index.spec.ts
Outdated
it('should work with files', async () => { | ||
const content = new Uint8Array([1, 2, 3]) | ||
const name = 'hello.txt' | ||
|
||
const hash = await bee.uploadFile(name, content) | ||
const file = await bee.downloadFile(hash) | ||
|
||
expect(file.name).to.equal(name) | ||
expect(file.data).to.deep.equal(content) | ||
}) | ||
|
||
it('should retrieve previously created empty tag', async () => { | ||
const tag = await bee.createTag() | ||
const tag2 = await bee.retrieveTag(tag) | ||
|
||
expect(tag).to.deep.include(tag2) | ||
}) |
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.
I was wondering if we can just run already define tests in the modules/*
but with the Bee instance instead. This seems like unnecessary duplication
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.
We can, however the point of this PR is that we no longer expose the modules to the public interface. Instead the Bee class is what we expose to the users of the library, so basically this is the API we are delivering. My intent was to write integration tests for that and also document how to use certain features. Imagine that later when features are removed from the go Bee client we may want to keep these functionalities but their implementation would change.
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.
As discussed in the call, the tension here is unnecessary replicating the test code. The proposed solution was to abstract the tests (e.g. similarly to how OpenZeppelin does it https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/test/token/ERC777/ERC777.test.js#L43-L59 )
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.
I would postpone this until we have the full Bee functionality and then see what could we test holistically. I imagine that we don't want to duplicate all the tests at the top-level but just make sure that at least they work on the happy path.
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.
Sounds good, let's do the test separation later.
b17b963
to
a59a3ee
Compare
48ed749
to
bb4868a
Compare
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.
LGTM
This PR adds the low-level module functionality to the
Bee
class and only exposes that. Therefore passing around the url is no longer necessary because it is encapsulated in the class instance.Also added error handling for axios responses and some helper types.
Fixes #5
The Buffers are replaced on the interface, but not internally (regarding #10)