- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.4k
 
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