-
Notifications
You must be signed in to change notification settings - Fork 26
fix(tests): Repair integration tests [fixes DXJ-506] #364
Conversation
@@ -22,6 +23,12 @@ const test = async () => { | |||
await symlink(CDN_PUBLIC_PATH, join(publicPath, "source")); | |||
} | |||
|
|||
try { |
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.
Can you please document what happens here and why?
Also, is it platform dependent in any way?
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 refactored it to self descriptive function.
Yes, it's platform independent
try { | ||
await access(join(publicPath, "deps")); | ||
} catch { | ||
await symlink(JS_CLIENT_DEPS_PATH, join(publicPath, "deps")); | ||
} |
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.
let's move that to a separate function.
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.
Will do
@@ -42,7 +41,7 @@ export interface CallAquaFunctionArgs { | |||
/** | |||
* Peer to call the function on | |||
*/ | |||
peer: IFluenceInternalApi; | |||
peer: unknown; |
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.
that's a little weird
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.
This should be refactored and moved to JS-client. Previous type was essentially the same. I will add TODO
to remember this.
@@ -14,7 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import type { Node } from "./commonTypes.js"; | |||
import type { Node } from "@fluencelabs/interfaces"; |
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.
Does that refer to a published library, or to a local path?
If it refers to a published library, you might have bad time updating these types.
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.
Moved it to js-client side.
export async function fetchResource( | ||
pkg: string, | ||
assetPath: string, | ||
root: string, |
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.
what is root? need some doc
export async function fetchResource( | ||
pkg: string, | ||
assetPath: string, | ||
root: string, |
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.
ditto
assetPath: string, | ||
root: string, | ||
) { | ||
// `root` will be handled somehow in the future. For now, we use filesystem root where js-client is running; |
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.
// `root` will be handled somehow in the future. For now, we use filesystem root where js-client is running; | |
// TODO: `root` will be handled somehow in the future. For now, we use filesystem root where js-client is running; |
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.
thanks
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 don't really understand what happens here.
What's the purpose of intefaces
library if it doesn't hold all the public interfaces?
@@ -85,7 +85,9 @@ const getRelayTime = () => { | |||
|
|||
const main = async () => { | |||
console.log("starting fluence..."); | |||
fluence.defaultClient = await fluence.clientFactory(relay, {}); | |||
fluence.defaultClient = await fluence.clientFactory(relay, { | |||
CDNUrl: "http://localhost:3000", |
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.
Will URL be reported in an error, if it is not available?
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.
It will give you runtime error
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.
JS-client will throw when you try to run it
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.
will the error message include URL? I think it should
My vision on @fluencelabs/interface is that is should be private in our ecosystem. It should contain very generic types for reusing between avm , marine-js, marine-worker, aqua-to-js and other packages in our ecosystem. Thus I dont see a reason to keep js-client's config type in there |
/** | ||
* Node of the Fluence network specified as a pair of node's multiaddr and it's peer id | ||
*/ | ||
export type Node = { |
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.
maybe rename since it's private now? like, Peer or something
No description provided.