-
Notifications
You must be signed in to change notification settings - Fork 2k
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
name-service-js: add tests #4013
Conversation
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.
This is really great work, thanks for your contribution! Just a few bits, then this should be good to go.
Can you also add a line to ci/js-test-name-service.sh
to do yarn test
? This way we can use these right away in CI! https://github.com/solana-labs/solana-program-library/blob/master/ci/js-test-name-service.sh is the file
e77a721
to
09f68a6
Compare
@joncinque how to build name service program before js-test. Make cargo-test-sbf run before js-test in CI? |
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.
The changes look good! Regarding the built program available to CI, you can see how other tests are doing it. Take a look at pull-request-token-swap.yml
as an example. You'll see that it uploads the program at the end of the cargo-test-sbf
step, and then it downloads the program during the js-test
portion. You'll need to add steps to do the same thing in pull-request-name-service.yml
Oh also, this is a little procedural nit from me -- when you make changes after a review, would you mind making those changes in new commits rather than amending your old commit? It makes it much easier to spot what you've done.
Looks great, thanks for your contribution! |
fixes #4004