-
Notifications
You must be signed in to change notification settings - Fork 150
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: integrate runtime-tests as a helper library #549
Conversation
run_process(["yarn"]) | ||
|
||
if chain == "polkadot": | ||
url = "wss://rpc.polkadot.io" |
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.
Would be cool if we had our own nodes - but I supposed you could not check the addresses into source control.
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 think we could input the URLs as either CLI args
or set env variables
for them instead.
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.
Yea I agree having our own nodes would be very nice. Would be a nice addition for the tools team in general.
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
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.
Great,
In another PR: would be super to have a job that runs this on chore(release)
PRs in the CI.
runtime-tests/index.ts
Outdated
* We return empty string's as an indication of a false value. JS recognizes "" as falsey. | ||
* NodeJS.Process.argv.reduce expects the return value of the accumulator to | ||
* be of type string, this is a way of satisfying the compiler, but also | ||
* returning the correct behavior. | ||
*/ |
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.
Not sure I understand what this does. What is the expected input here? It walks all cli arguments, filters out anything that starts with "/", sets the config[cmd]
as it goes? How is it used?
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.
Yea this can be a bit confusing.
-
Parses out all inputs that start with
/
. (This is so we dont save things such as the path of the node.js binary into the config) -
For everything else If the input doesn't start with
--
, then we return""
(This sets the accumulator in this casecmd
to false). This will resolve to a falsey value which will stop the reducer from saving it into the config. -
argv.push(arg)
is used to save any local config options in order to pass it into the jest binary such as--config=./runtime-tests/jest.config.js
. -
If the
arg
is equal to--
and has the name ofchain
then it will set it equal to the accumulator orcmd
and then the arg that follows will be the argument for the command itself.
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.
Hmm, I wonder if we're trying to be a bit too clever for our own good here; feels like this should be easier to do? ("no it's just you being thick" is a valid answer here!).
I was actually wondering something more basic: what is this args processing for? Is this right (or close to right)?
"Process the command line arguments used to launch the test session. Merge any config options provided at the command line with the on disk config."
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.
Yea so you actually make a great point and I was able to incredibly simplify it. This method is way too clever for what it is suppose to be. I basically just wrote out a custom parser which doesn't need to be done either.
I chose to do it this way because there is no straight forward way to pass custom CLI commands to jest other than setting an entry file, and calling the binary after adding a custom configuration to the disk. But that being said I also could have just used a arg-parser and then directly pass it in same as I did. Which is exactly what I changed it too. I added argparse
now and have it just be the same as any other node argument parsing tool. No loops, or complicated logic anymore.
Good catch. Thanks
runtime-tests/index.ts
Outdated
} | ||
} | ||
|
||
argv.push(arg); |
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'm used to seeing reduce()
return the accumulator at every iteration (including the last) and assign the return value to a variable, e.g. let sum = [1,2,3].reduce((acc, x) => x*x)
. Instead here it seems like you're not using the return value of the whole reduce
and relying instead on side-effects like this one. Should it be a for-loop then?
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.
So in this case I am using the reducer a little differently than normal, the accumulator in this case is being used to identify falsey values, vs truthy commands. I stayed away from the for loop
because I didnt want to introduce messy iterator manipulation in order to parse the args, and I think the reducer itself allows for more customization for the future in case we wanted to add arguments to the script.
…into tarik-add-runtime-tests
This reverts commit 6f2c253.
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@emostov Before we merge this, can you pull this branch down and test two things as a sanity check as well.
|
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
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
For follow up based on convos
- Scan sidecar process stdout on startup to tell if sidecar is connected to the archive node before starting tests (right now we have a 30sec timeout)
- Save response from each endpoint and compare those to the responses received while the test is running. We can use these to document changes to the user api contract
@TarikGul Ran it locally and it all checks out ✅ |
Ready to merge. Per my conversation with @emostov, we think it could be a great idea to turn this runtime-tests into a full e2e test for the Part 2Currently the runtime tests only checks to make sure we receive a successful response back from the node. I do this by checking the block height of the response against the expected block height. But this could be done better by checking against a full response. Therefore simulating a full user request-response cycle. This would mean efficiently storing the endpoints we currently have logged for each runtime per chain, and then matching the JSON responses from the node against our stored responses. |
|
||
argv.push(arg); | ||
return ''; | ||
const parser = new ArgumentParser(); |
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.
❤️
…into tarik-add-runtime-tests
closes: #495
This integrates sidecar-runtime-test into Substrate-api-sidecar. I also used this script written by @niklasad1 to help automate the tests for each chain.
TODO: