-
Notifications
You must be signed in to change notification settings - Fork 9
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
SYS-334 - Testing and Coverage Report Generation #47
base: dev
Are you sure you want to change the base?
Conversation
* add new test cases * clean up tests
* fixes race condition in clearing stats collection object causing skewed rpc api call numbers * Makes saveInterfaceStat clear apiPerfLogData before kicking off async DB write to prevent double counting stats
* Add new transaction test cases * add test files
* Remove gitlab instances from readme * fix linting and add community section
package.json
Outdated
@@ -4,15 +4,17 @@ | |||
"description": "JSON RPC server for Shardeum", | |||
"main": "src/server.js", | |||
"scripts": { | |||
"test": "echo \"Error: no test specified\" && exit 1", | |||
"test": "NODE_ENV=test jest --detectOpenHandles --silent --forceExit", |
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.
@kenny-io does this work without running the run_tests.sh script?
I assume if you just straight up run jest it'll error out because there's no network available for it to connect to
As our build pipelines call npm run test for where its available, I think "working without additional steps or intervention" might be a good goal here
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 really good call, the test command was supposed to be for folks (like internal team members) who may already have a locally running Shardeum network and want to test without the run_tests script.
@@ -0,0 +1,7 @@ | |||
{ | |||
"presets": [ |
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 was the source for this?
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.
@paulgs9988 added the file with this commit f289260
@@ -4,15 +4,16 @@ | |||
"description": "JSON RPC server for Shardeum", | |||
"main": "src/server.js", | |||
"scripts": { | |||
"test": "echo \"Error: no test specified\" && exit 1", |
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.
@kenny-io why was this removed? npm test
is going to be expected to be here for the github workflow automation
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.
Oh yes Chris pinged me earlier about it. I've update it to run the test script in run_tests.sh
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.
Following these guidelines: https://docs.google.com/document/d/1At2lU0hYOsVcUeWo8AmVdWAup2hwq6HrVikcVstpX_Q/edit#heading=h.qrk5audklacg
Is anyone opposed to my renaming the existing test directory? I just want to make sure that nothing else is dependent on this naming convention ATM.
This is a very comprehensive PR which seems to consolidate work from various other branches/PRs including the "code-coverage" branch and the following PR: #43. I'm hoping someone else might be able to update this with a more comprehensive description, but I've mostly just added some configuration stuff in order to easily generate coverage report artifacts to be generated by Jenkins.
There are many tests that have had several refactors included in this additionally, found in the src>tests directory, in addition to other contributions which far exceed mine.
My goal is to get this thoroughly reviewed and merged however since the various git branches are making the automation implementation unnecessarily messy at the moment.