Replace bash test to TS using shelljs #251

Nathan-SL · 2022-11-08T14:40:45Z

Usage related changes

Closes Replace bash tests with JS #248
Change check.sh shell scripts to TS using Shelljs

Development related changes

Tests written inside test_case/check.sh have been removed and instead test_case/check.ts will be used.
Tests are now initiated using npx mocha

Checklist:

Formatted the code
No linter errors + tried to avoid introducing linter warnings
Performed a self-review of the code
Rebased to the last commit of the target branch (or merged it into my branch)
Documented the changes
Updated the test directory (with a test case consisting of network.json, hardhat.config.ts, check.sh)
Linked issues which this PR resolves
All tests are passing (for external contributors who don't have access to the CI/CD pipeline)

FabijanC

No need to directly use shelljs for something that's natively supported in node.js (cp, rm, mv, potentially others)

Also, when a check.ts file is introduced, the corresponding check.sh can be deleted. I think it would be easier to review if both were visible at the same time in the Files changed part of the PR.

test/configuration-tests/with-account-compilation-option/check.ts

- Use console.log in place of shell.echo - use fs functions to copy, delete and read file

- Use spawn to detach bg process - add --no-compile flag

Nathan-SL · 2022-11-15T11:51:12Z

Here I've used exec("cp ...") instead of fs.copyFileSync() due to fs.copyFileSync() failure for archive mode copying.

FabijanC · 2022-11-16T08:45:43Z

How much trouble would it be to also convert test.sh to e.g. TS? Doesn't have to be done in this PR, just asking for a future improvement.

scripts/test.sh

test/configuration-tests/with-account-compilation-option/check.ts

test/utils/utils.ts

test/recompilation-tests/hardhat-test/check.ts

test/configuration-tests/with-sources-path/check.ts

test/general-tests/postman/check.ts

test/integrated-devnet-tests/with-stderr-to-STDERR/check.ts

test/integrated-devnet-tests/with-stderr-to-file/check.ts

- Remove shell.exit, shell.echo - Use axios inplace of curl - Update contains function

Nathan-SL · 2022-11-22T13:16:30Z

How much trouble would it be to also convert test.sh to e.g. TS? Doesn't have to be done in this PR, just asking for a future improvement.

I'd say not much trouble. It can be handled in a similar fashion like what this PR achieves.

FabijanC

Some conversations were resolved (even though no commit link was provided) so I marked them as resolved. But some I left unresolved since they don't look addressed to me.

StarknetPluginError in tests should be replaced with some kind of assertion error (it can be explicitly thrown, or implicitly - through e.g. chai expectations).

test/utils/deploy-funded-account.ts

test/utils/utils.ts

test/general-tests/starknet-verify/check.ts

test/utils/cli-functions.ts

test/general-tests/postman/check.ts

test/general-tests/hardhat-run/check.ts

scripts/test.sh

Nathan-SL · 2022-11-24T09:21:07Z

Some conversations were resolved (even though no commit link was provided) so I marked them as resolved. But some I left unresolved since they don't look addressed to me.

StarknetPluginError in tests should be replaced with some kind of assertion error (it can be explicitly thrown, or implicitly - through e.g. chai expectations).

f4cedb1

test/utils/deploy-funded-account.ts

test/utils/utils.ts

- Use assertContains - Throw AssertionError

test/utils/utils.ts

Nathan-SL added 4 commits November 8, 2022 17:34

WIP: Replace bash test to TS using shelljs

c6fe5ca

Add no-compile flag to test

b43adda

Put no-compile after the task name

701a9a2

Update suits for configuration test set

46201c4

Nathan-SL requested a review from FabijanC November 9, 2022 10:57

FabijanC requested changes Nov 9, 2022

View reviewed changes

test/configuration-tests/with-account-compilation-option/check.ts Outdated Show resolved Hide resolved

test/configuration-tests/with-account-compilation-option/check.ts Outdated Show resolved Hide resolved

Nathan-SL added 10 commits November 14, 2022 14:14

Remove check.sh files

189a496

Replace check.sh with check.ts

77f9a40

- Use console.log in place of shell.echo - use fs functions to copy, delete and read file

Make venv files executable

2c56e9e

Update test call to mocha

a0cc8c1

Update test

d116760

- Use spawn to detach bg process - add --no-compile flag

Terminate detached processes

0e3bcba

Terminate detached processes

2ca78f0

Update starknet-devnet version

c904bb9

Merge branch 'master' into replace-bash-to-shelljs

f4472c9

Update dev docs [skip ci]

96def07

FabijanC requested changes Nov 16, 2022

View reviewed changes

Nathan-SL added 6 commits November 17, 2022 10:48

Remove prev comment from test.sh

6c2ef52

Use path.join in place of concat

90d81ba

Update utils

aa04ee4

- Remove shell.exit, shell.echo - Use axios inplace of curl - Update contains function

Have create-fund-deploy script

58238d7

Add starknet cli helper functions

5ff203a

Update test based on comments

7a6cc5a

Nathan-SL added 4 commits November 22, 2022 16:39

🔥 Remove unused scripts & update utils

1ae0f61

Remove repetitive log [skip ci]

9ea524d

Change branch

c949138

Kill with process.kill() & avoid silent execute

e0a32dd

Nathan-SL added 3 commits November 23, 2022 09:18

Bug fix with contract path

0ef112a

Use kill from shell

ce0b6e9

Remove unused variable

04c64f4

FabijanC requested changes Nov 23, 2022

View reviewed changes

Nathan-SL added 9 commits November 23, 2022 22:31

Fix typo

26185a0

Avoid use of $0 in ts tests

f053778

Restore branch name

7f1c800

Use constants for default url

3bd3c03

Read port from constant

fb0c77a

Replace StarknetPluginError with assertion

f4cedb1

Add new methods

7f9318d

Minor clean up

bcf5eec

Update import

7769e97

FabijanC requested changes Nov 24, 2022

View reviewed changes

test/utils/deploy-funded-account.ts Outdated Show resolved Hide resolved

test/utils/utils.ts Outdated Show resolved Hide resolved

test/utils/utils.ts Outdated Show resolved Hide resolved

test/utils/utils.ts Outdated Show resolved Hide resolved

Nathan-SL added 4 commits November 24, 2022 22:13

Update test

f869821

- Use assertContains - Throw AssertionError

Silent venv activation

4d34851

Remove unused imports

5ef56b8

Remove venv files [skip ci]

218c3e7

FabijanC requested changes Nov 25, 2022

View reviewed changes

test/utils/utils.ts Outdated Show resolved Hide resolved

Nathan-SL added 2 commits November 25, 2022 11:38

Multiline string [skip ci]

7755821

Format and use multiline error message [skip ci]

a34e110

FabijanC approved these changes Nov 25, 2022

View reviewed changes

FabijanC merged commit 3183645 into master Nov 25, 2022

FabijanC deleted the replace-bash-to-shelljs branch November 25, 2022 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace bash test to TS using shelljs #251

Replace bash test to TS using shelljs #251

Nathan-SL commented Nov 8, 2022 •

edited

FabijanC left a comment

Nathan-SL commented Nov 15, 2022 •

edited

FabijanC commented Nov 16, 2022

Nathan-SL commented Nov 22, 2022

FabijanC left a comment

Nathan-SL commented Nov 24, 2022

Replace bash test to TS using shelljs #251

Replace bash test to TS using shelljs #251

Conversation

Nathan-SL commented Nov 8, 2022 • edited

Usage related changes

Development related changes

Checklist:

FabijanC left a comment

Choose a reason for hiding this comment

Nathan-SL commented Nov 15, 2022 • edited

FabijanC commented Nov 16, 2022

Nathan-SL commented Nov 22, 2022

FabijanC left a comment

Choose a reason for hiding this comment

Nathan-SL commented Nov 24, 2022

Nathan-SL commented Nov 8, 2022 •

edited

Nathan-SL commented Nov 15, 2022 •

edited