Http api 1.0 rc changes #39

TimDaub · 2017-06-19T12:41:42Z

Changes the Connection model to the agreed on HTTP API breaking changes as outlines here
adds tests for all changes
adds sinon.js for mocking
changed path describing keys to camelCase
added constant.js (similar to conftest.py)
added integration tests that run against bdb
updated ascribe-eslint package
test and fix asset endpoint
test and fix outputs endpoint

More info:

Fixes For connection.js, in all methods use camelCase named parameters (and preferably no objects) #48

TimDaub · 2017-06-20T11:50:02Z

test/connection/test_connection.js

+
+
+// TODO: Find out how to mock `request`
+test.skip('Request with custom headers', t => {


Import/export from parent module (index.js)

TimDaub · 2017-06-20T15:49:16Z

Add integration test for asset endpoint :diamond:

:(

TimDaub · 2017-06-20T15:53:41Z

Please note that in Connection.js I broke the interface to use camelCased parameters for some methods already.

For an example, see this:

https://github.com/bigchaindb/js-bigchaindb-driver/pull/39/files#diff-cb3b6e63284fb91cd988a927d0ddc7c6R85

~~It will require me to fix the rest of those before releasing a 1.0 compatible release.~~

I just finished this with de072dc and 9191745.

codecov-io · 2017-06-21T11:33:18Z

Codecov Report

Merging #39 into master will increase coverage by 18.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #39       +/-   ##
==========================================
+ Coverage   50.53%   68.7%   +18.16%     
==========================================
  Files          21      21               
  Lines         279     278        -1     
==========================================
+ Hits          141     191       +50     
+ Misses        138      87       -51

Impacted Files	Coverage Δ
...saction/serializeTransactionIntoCanonicalString.js	`100% <ø> (ø)`	⬆️
src/connection/index.js	`93.54% <100%> (+65.42%)`	⬆️
src/baseRequest.js	`75% <0%> (+21.42%)`	⬆️
src/format_text.js	`36.36% <0%> (+27.27%)`	⬆️
src/stringify_as_query_param.js	`90% <0%> (+60%)`	⬆️
src/sanitize.js	`73.68% <0%> (+63.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cde6df...1aa430a. Read the comment docs.

vrde

Apart from the first test in test/connection/test_connection.js, I'm not sure I understand how the rest of the tests in the same file work.

The file test/constants.js contains a mixture of constants and functions that returns variable values. I think hard-coding Alice's pubkey (and outputs) is not a good idea since tests run in parallel. In this case we can have problems processing the unspents—e.g. I want to create a test where I transfer something from Alice to Bob. If Bob's pubkey is constant, tests can influence each others and assertions like:

unspents_t-1(Bob) = unspents_t(Bob) + 1

can easily fail.

vrde · 2017-06-20T15:56:58Z

src/connection/index.js

@@ -8,16 +8,15 @@ export default class Connection {
    }

    getApiUrls(endpoints) {


endpoints → endpoint (singular)

vrde · 2017-06-20T16:01:20Z

src/connection/index.js

@@ -128,12 +133,10 @@ export default class Connection {
            const timer = setInterval(() => {
                this.getStatus(txId)
                    .then((res) => {


remove extra parenthesis 💅

vrde · 2017-06-20T16:01:46Z

src/connection/index.js

@@ -128,12 +133,10 @@ export default class Connection {
            const timer = setInterval(() => {
                this.getStatus(txId)
                    .then((res) => {
-                        console.log('Fetched transaction status:', res) // eslint-disable-line no-console
                        if (res.status === 'valid') {
                            clearInterval(timer)
                            this.getTransaction(txId)
                                .then((res_) => {


remove extra parenthesis 💅

vrde · 2017-06-20T16:02:16Z

test/connection/test_connection.js

+        'transactionsDetail': 'transactions/%(transactionId)s',
+        'assets': 'assets',
+    }
+    Object.keys(endpoints).forEach((endpointName) => {


remove extra parenthesis 💅

vrde · 2017-06-21T11:34:30Z

test/constants.js

+// TODO: Find out if ava has something like conftest, if so put this there.
+
+// NOTE: We cast `Math.random()` to a string, as sometimes Javascript simply
+// yields a slightly different float during runtime, lol


What do you mean by "slightly different float"?

TimDaub · 2017-06-21T13:36:08Z

Apart from the first test in test/connection/test_connection.js, I'm not sure I understand how the rest of the tests in the same file work.

They check if for given input parameters, the tested function calls the request method correctly.
For example click here. Note that a boolean is passed to the function, but a string comes out.

I think hard-coding Alice's pubkey (and outputs) is not a good idea since tests run in parallel. In this case we can have problems processing the unspents—e.g. I want to create a test where I transfer something from Alice to Bob. If Bob's pubkey is constant, tests can influence each others and assertions like:

unspentst-1(Bob) = unspentst(Bob) + 1
can easily fail.

OK, I can fix this.

TimDaub · 2017-06-21T14:02:30Z

remove extra parenthesis 💅

ascribe/javascript#56

vrde · 2017-06-21T14:10:05Z

test/integration/test_integration.js


    const createTx = Transaction.makeCreateTransaction(
        asset(),
        metaData,
-        [aliceOutput, aliceOutput],
-        alice.publicKey
+        [carolOutput, carolOutput],


who is Carol?

ssadler · 2017-06-21T14:29:56Z

Why is it connection/index.js rather than just connection.js?

TimDaub · 2017-06-21T15:50:42Z

Why is it connection/index.js rather than just connection.js?

#57

TimDaub requested review from ssadler and vrde June 19, 2017 12:42

TimDaub force-pushed the http-api-1.0-rc-changes branch 7 times, most recently from c747533 to 9a73fc1 Compare June 20, 2017 09:06

TimDaub commented Jun 20, 2017

View reviewed changes

TimDaub force-pushed the http-api-1.0-rc-changes branch from 9a73fc1 to 5c92661 Compare June 20, 2017 15:47

tim added 12 commits June 21, 2017 13:27

Adjust HTTP API endpoints and test

af49ec1

CamelCase for path keys

d4ea4d6

Test Connection._req

d8cb13b

Add constants.js

9bc0773

Add integration tests

03c17ef

serializeTransactionIntoCanonicalString line length

44896b9

Comply to eslint 🔫

3fdaf40

Add integration test for asset endpoint :diamond:

db10937

Fix and test outputs endpoint

8bd602f

Test list blocks for a transaction id

3671c57

Test list transaction for an asset id

1696f7c

endpoints => endpoint

b883e1f

TimDaub force-pushed the http-api-1.0-rc-changes branch from e1d0f21 to b883e1f Compare June 21, 2017 11:28

vrde suggested changes Jun 21, 2017

View reviewed changes

Dynamically generated keys for /outputs tests + bool fix

c17d614

TimDaub mentioned this pull request Jun 21, 2017

If no parenthesis in shorthard fn declaration possible ascribe/javascript#56

Open

rm ()

0da90e8

vrde reviewed Jun 21, 2017

View reviewed changes

vrde approved these changes Jun 21, 2017

View reviewed changes

Adjust float interpretation comment

1aa430a

ssadler approved these changes Jun 21, 2017

View reviewed changes

TimDaub merged commit b8d73b2 into master Jun 21, 2017

kremalicious deleted the http-api-1.0-rc-changes branch June 28, 2017 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http api 1.0 rc changes #39

Http api 1.0 rc changes #39

TimDaub commented Jun 19, 2017 •

edited

Loading

TimDaub Jun 20, 2017

TimDaub Jun 20, 2017

TimDaub commented Jun 20, 2017 •

edited

Loading

TimDaub commented Jun 20, 2017 •

edited

Loading

codecov-io commented Jun 21, 2017 •

edited

Loading

vrde left a comment •

edited

Loading

vrde Jun 20, 2017

vrde Jun 20, 2017

vrde Jun 20, 2017

vrde Jun 20, 2017

vrde Jun 21, 2017

TimDaub commented Jun 21, 2017 •

edited

Loading

TimDaub commented Jun 21, 2017

vrde Jun 21, 2017

ssadler commented Jun 21, 2017

TimDaub commented Jun 21, 2017



		// TODO: Find out how to mock `request`
		test.skip('Request with custom headers', t => {

		@@ -8,16 +8,15 @@ export default class Connection {
		}

		getApiUrls(endpoints) {

Http api 1.0 rc changes #39

Http api 1.0 rc changes #39

Conversation

TimDaub commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub commented Jun 20, 2017 • edited Loading

TimDaub commented Jun 20, 2017 • edited Loading

codecov-io commented Jun 21, 2017 • edited Loading

Codecov Report

vrde left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub commented Jun 21, 2017 • edited Loading

TimDaub commented Jun 21, 2017

Choose a reason for hiding this comment

ssadler commented Jun 21, 2017

TimDaub commented Jun 21, 2017

TimDaub commented Jun 19, 2017 •

edited

Loading

TimDaub commented Jun 20, 2017 •

edited

Loading

TimDaub commented Jun 20, 2017 •

edited

Loading

codecov-io commented Jun 21, 2017 •

edited

Loading

vrde left a comment •

edited

Loading

TimDaub commented Jun 21, 2017 •

edited

Loading