Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

RPC test method cleanup #126

Merged
merged 1 commit into from
Jun 9, 2020
Merged

RPC test method cleanup #126

merged 1 commit into from
Jun 9, 2020

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Jun 8, 2020

This PR simplifies & wraps common code parts of the RPC test methods since there is so much code redundancy which will likely get a bit out of hand if we continue with so much copy-and-paste.

Have done this for one file only (getBlockByHash) for now and would apply to all RPC test files after some feedback here.

@coveralls
Copy link

coveralls commented Jun 8, 2020

Coverage Status

Coverage remained the same at 92.684% when pulling 4cd3a31 on rpc-test-method-cleanup into f948aff on master.

@ryanio
Copy link
Contributor

ryanio commented Jun 8, 2020

this is great! yes this will make life a lot easier

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

LGTM!

@holgerd77 holgerd77 force-pushed the rpc-test-method-cleanup branch from ad35ba2 to 6db573a Compare June 9, 2020 09:12
@holgerd77 holgerd77 force-pushed the rpc-test-method-cleanup branch from 6db573a to 4cd3a31 Compare June 9, 2020 09:17
@holgerd77
Copy link
Member Author

Ok, have done, this is now ready for re-review.

Have also added additional pass checks, since this test-is-implicitly-passed-if-no-error-thrown logic made me to nervous at the end, since one can just not judge if the test is running at all.

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

looks great, awesome cleanup

@holgerd77 holgerd77 merged commit fd76683 into master Jun 9, 2020
@holgerd77 holgerd77 deleted the rpc-test-method-cleanup branch June 9, 2020 18:25
Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

This PR got merged while I was reviewing it 😅! Nice cleanup, though.

Noticed that there are more assertions as well. From 230 to 280. 👌

It have just a little adjustment for test semantics (that's really not a big deal, just a nitpick), regarding the use of throw new Error(…). It does not show the expected/received values and we have to do it manually:

function compareErrorCode (t, error, errorCode) {
  const msg = `should return the correct error code (expected: ${errorCode}, received: ${error.code})`
  if (error.code !== errorCode) {
    throw new Error(msg)
  } else {
    t.pass(msg)
  }

image

Using t.equals(…), besides being cleaner, we automatically get the expected and received values, so there's no need to interpolate the string with them:

function compareErrorCode (t, error, errorCode) {
  const msg = `should return the correct error code`
   t.equals(error.code, errorCode, msg)
 }

image

@holgerd77
Copy link
Member Author

Thanks, makes sense, I will eventually place this along an another-topic-PR (or someone else can do to). Hopefully I will remember. 😄

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants