Adds rest of fetch endpoint functions
#26
Conversation
|
👌 |
…into feature/local
| * Request type of /send endpoint. | ||
| * | ||
| * @param cmds - Non-empty array of Pact commands (or transactions) to submit to server. | ||
| */ |
There was a problem hiding this comment.
I think I've added semver modifiers (@Alpha beta private etc), so they might be there when you rebase.
| } | null; | ||
| } | null; |
There was a problem hiding this comment.
null types are discouraged. They can be replaced by undefined unless they're needed by the API. Then we can parse the undefined to null
There was a problem hiding this comment.
The current API needs this. We need to discuss this to alter the current API I don't like null either.
| txId: number | null; | ||
| result: PactResultSuccess | PactResultError; | ||
| gas: number; | ||
| logs: string | null; |
rush.json
Outdated
| { | ||
| "packageName": "@kadena/integration-tests", | ||
| "projectFolder": "packages/libs/integration-tests", | ||
| "tags": ["libs"], |
There was a problem hiding this comment.
Maybe this is more of a "tools" type of package?
There was a problem hiding this comment.
I changed the tag to tools. Should I also move it into the tools subdirectory in /packages?
There was a problem hiding this comment.
Yes. Related: maybe we don't need the packages directory and can have the subdirs in the root? What do you think? Packages is common when using a monorepo with sub-packages, but I'm not sure if it has any added value for us
There was a problem hiding this comment.
using packages gives a bit more overview IMHO if we are going to add dapps in here as well
Co-authored-by: Albert G <516972+alber70g@users.noreply.github.com>
| @@ -39,6 +37,7 @@ | |||
| "@types/heft-jest": "~1.0.3", | |||
| "@types/jest": "^27.5.1", | |||
| "@types/node": "^16.0.0", | |||
| "concurrently": "^7.2.2" | |||
| "concurrently": "^7.2.2", | |||
| "exponential-backoff": "~3.1.0" | |||
There was a problem hiding this comment.
Is there a specific reason to depend on the ~ only patch updates of this package? Otherwise we might change this to ^. I believe rush install -p <package> in the package directory does that automatically?
Also, I think this should not be a devDependency, as it's needed during the runtime of the package (i.e. running tests)
| describe('[DevNet] Makes /send request to initiate a cross-chain transaction', () => { | ||
| test('Receives request key of transaction', async () => { | ||
| const actual: SendResponse = await send(sendReq2, devnetApiHostChain0); | ||
| const expected: SendResponse = { | ||
| requestKeys: [signedCommand2.hash], | ||
| }; | ||
| expect(actual).toEqual(expected); |
There was a problem hiding this comment.
edit: I see further down that you did add multiple its in one describe, so maybe just ignore this comment if it's intentional
I see you've added describe to all the tests individually. I'm not sure if my explanation was clear enough, that there can be multiple its in one describe. E.g.:
describe('subject under test', () => {
it('behaves in a certain way', () => {});
it('behaves in another way with other variables', () => {});
})`.
I'd say subject under test would here be devnet?
And one of the behaviours would be receives request key, on /send request of simple transaction
And another would be: receives the expected transaction result, on /local request of simple transaction
(see attachSignature.test.ts and base64UrlEncode.test.ts )
I'm not yet familiar enough with the context, so this might be semantically incorrect?
| export function message(msg: string): string { | ||
| return `Hello, ${msg}!`; | ||
| } |
There was a problem hiding this comment.
This one can be removed. I think it's from a boilerplate or something?
There was a problem hiding this comment.
yes shall we remove this in another PR as it's currently also in the repo
| import { message } from '../index'; | ||
|
|
||
| test('check if message add the text hello', () => { | ||
| expect(message('Kadena')).toEqual('Hello, Kadena!'); |
There was a problem hiding this comment.
Same here, we don't need this test
CommandResulttype/localfunction/pollfunction/listenfunction/spvfunction/localintegration test/localunit test (using integration test results)/listenintegration (devnet and pactserver) and unit tests/pollintegration (devnet and pactserver) and unit tests/spvintegration (devnet and pactserver) and unit testsNOTE:
There is some indeterminism in the integration tests due to devnet's slow block production.
Increasing the sleep time before listening for a result usually fixes the errors.
I've created an issue here to track this work: #33
UPDATE: Jul 8, 2022
Added exponential backoff and now tests complete more consistently.