Auto tests #45
Auto tests #45
Conversation
tests/.vscode/launch.json
Outdated
tests/contracts/basetests.ts
Outdated
| } | ||
| }, | ||
| error => { | ||
| resultOk = false |
There was a problem hiding this comment.
This continues executing even if the result is false. Why not use Promises.all
There was a problem hiding this comment.
Actually, we may want to introspect when something fails, so we might prefer to use Promises.allSettled
tests/contracts/basetests.ts
Outdated
| this.contractId = data.toString().trim() | ||
| }, | ||
| error => { | ||
| result = false |
There was a problem hiding this comment.
Just return the promise and handle the result upstream
tests/contracts/raffletests.ts
Outdated
| } | ||
|
|
||
| async getTokenId() { | ||
| let resultOk = true |
There was a problem hiding this comment.
Same here, return the promise itself instead of bool
tests/contracts/raffletests.ts
Outdated
| } | ||
|
|
||
| async invokeBuyTickets() { | ||
| let resultOk = true |
There was a problem hiding this comment.
Same here with Promises.all
tests/contracts/raffletests.ts
Outdated
| } | ||
|
|
||
| async invokePlayRaffle() { | ||
| let resultOk = true |
tests/contracts/votingtests.ts
Outdated
| await this.calculateFeesForAccount('player_1') | ||
| } | ||
|
|
||
| public async run(): Promise<boolean> { |
There was a problem hiding this comment.
If these methods return promises then you can do:
this.createAccounts()
.then(res => this.deployContract('voting'))
.then(......)
.catch((e) => ... )
tests/testsrunner.ts
Outdated
| const test = this.tests.get(testName) | ||
| if (test === undefined) { | ||
| console.error(`Could not find test ${testName}`) | ||
| --testsToRun |
There was a problem hiding this comment.
| --testsToRun | |
| throw new Error(`Could not find test ${testName}`) |
There was a problem hiding this comment.
That would cause the app to exit which I don't want. Let it still continue with other found tests.
There was a problem hiding this comment.
I think the problem inherently comes from having a custom test runner.
Have you considered something like TestRunner?
It would help run tests per file, this gives the person running tests ability to control what someone runs.
WDYT?
There was a problem hiding this comment.
I have not. Will take a look at it later. The implemented test runner is very simple and I'm not sure there's any need to change it or make it more complex :)
There was a problem hiding this comment.
I think you are reinventing the wheel.
Eg I can't run a single test with the current implementation, I have to run all or change the code.
Test runner comes bundled with node and is well documented and tested. I don't think that is complexity as compared to having to reference a custom impl. The changes requested are not major, you just have to wrap the calls eg:
test('Raffle Contract', async (t) => {
await t.test('init' , (t) => {
assert.strictEqual(1, 1);
});
await t.test('create_proposal;', (t) => {
assert.strictEqual(2, 2);
});
}); Now:
- I can run each file alone.
- The test runner manages what fails etc.
- The test runner can output other file formats if needed.
If you are not into working on it, I can do it because I don't feel ok with the current impl.
There was a problem hiding this comment.
There's nothing more to manage than in the example you've posted. You still have to write at least the same amount of code. I don't see a value added here.
There was a problem hiding this comment.
I think I disagree with the notion that it adds no value.
First of all we wouldn't have the testrunner file, secondly when adding new contracts we don't need to declare a new contract in code, just add the file and it runs.
We plan to have up to 10 contracts, I think I see a lot of value in this.
I can do a PR with the changes I am recommending if need be.
There was a problem hiding this comment.
Ok, feel free to add it.
There was a problem hiding this comment.
I think this is a tool we should prepare for external use. I see both POVs. But i think the developer experience will be better with a well known test runner. Also this test runners normally have the possiblity of multiple output formats, which can be used later in a miriad of ways :)
|
Are these tests run by CI? |
tests/appconfig.ts
Outdated
| import configJson from './config.json' | ||
|
|
||
| export class AppConfig { | ||
| public friendbot: string = '' |
There was a problem hiding this comment.
| public friendbot: string = '' | |
| public friendbot: string = configJson.server.friendbot; |
Is this possible? We would not need the parseConfig().
There was a problem hiding this comment.
Hello ! thanks for PR description. I am still reading the code in detail, just providing early feedback here:
-
Would it be good to add a
README.mdinside thetestfolder, indicating what does this tool and some examples ? Imagine someone with no context lands in that folder, what would be useful ? Also, should we point to this README.md from the root one ? -
Its sad we cannot implement this in Rust to be honest, taking into account we are targeting Soroban smart contracts (i know, nothing to do with contracts themselves, but the community writes things in rust). I would like to know how far we would be from this and what we would need from Soroban to implement/fix. Maybe its something doable to open an upstream issue :)
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Should we close this PR without merging now when the |
|
Lets close this one. |
|
There is another automated tests tool written using |
Referenced issue
Closes #34
Summary of changes
Here's a simple node.js tool for automated contract tests. What it does:
To run it call
npm run tests. You can specify which tests to run, e.g.npm run tests voting. If any of the test fails, tool will exit with -1 code. Tool could be added to GitHub jobs as an action run after each commit.Each contract has its separate ts code with the test scenario. To add new test for a new contract just create new ts file, extend the
BaseTestsclass and updateTestsRunnerclass.The
Raffletest is not completed yet, as the contract fails onfuturenet.Reviewer recommendations
How to make the review easier. i.e "Its a big PR and the recommendation is to go through each commit"
or "In order to run the tests, execute ..."