Skip to content
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

CI: add a test case for create account #44

Merged
merged 15 commits into from
Jun 8, 2019
Merged

Conversation

janedegtiareva
Copy link
Contributor

No description provided.

@@ -8,3 +8,8 @@ npm install
npm uninstall near-shell
npm install ../
NODE_ENV=development npm run test
timestamp=$(date +%s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put this into separate script, e.g. test/create_account.sh?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe have a setup script which sets up new project and then run few different scenarios in separate scripts after that:

  • run tests
  • create account
  • deploy

package.json Outdated
@@ -6,7 +6,7 @@
"scripts": {
"build": "echo \"Error: not implemented\" && exit 1",
"deploy": "echo \"Error: not implemented\" && exit 1",
"test": "./test/new_project.sh"
"test": "./test/new_project.sh; ./test/create_account.sh"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to use && instead of ; here, otherwise npm might not return error code on ./test/new_project.sh failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it with a purposefully invalid account id, and it does in fact fail the test with ;

&& does not work at all however.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bash-3.2$ false ; true
bash-3.2$ echo $?
0
bash-3.2$ false && true
bash-3.2$ echo $?
1
bash-3.2$ true ; false
bash-3.2$ echo $?
1

I just tested it with a purposefully invalid account id, and it does in fact fail the test with ;

it fails because last step failed. It won't fail if ./test/new_project.sh fails

&& does not work at all however.

may you elaborate? It does work as expected in bash (see above). Maybe ./test/new_project.sh returns error somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAIL src/test.js
Test suite failed to run

InternalServerError: Internal Server Error
  
  at sendJson (../node_modules/nearlib/internal/send-json.js:12:15)

I'd prefer to not spend cycles to investigate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to not spend cycles to investigate

so our goal is to have npm run test exit with error code when either of test scripts fails.
Using ; doesn't achieve this (see bash transcript above), npm run test would be successful if ./test/new_project.sh fails.
I think it's reasonable requirement that npm run test should fail when ./test/new_project.sh fails.

Just to be sure I checked that npm behavior is the same by modifying test script to be false; true:

➜  nearlib git:(master) ✗ npm run test && echo SUCCESS                                                                                                                                              <<<

> nearlib@0.7.1 test /Users/vg/Documents/nearlib
> false; true

SUCCESS

I'd prefer to not spend more cycles explaining why this is a bug. I can of course also fix it myself, similar like I had to do to near-shell (e9ce691) cause CI would be completely happy with failed deploy, etc otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time we get "Internal Server Error" without any details - that's a good indicator to actually investigate. Because that means that developer using this will be totally lost with this error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for sure "&&" should be used in any bash environment like testing configs (note, in fish && doesn't work)

@@ -0,0 +1,6 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to do set -ex first (see more here https://explainshell.com/explain?cmd=set+-ex). -x is kinda matter of situation, but without -e script won't fail CI properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@vgrichina vgrichina merged commit 02958c2 into master Jun 8, 2019
@volovyks volovyks deleted the j-createaccounttest branch October 19, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants