Lisk-sdk: The makeRequest function in swagger_spec makes tests harder to read

[jondubois]

Expected behavior

The makeRequest function (which we use heavily throughout the test suites) should make an HTTP request without changing what is passed as the parameters argument. We shouldn't have to open the swagger_spec.js file to understand what a test case is doing.

• Ideally, the HTTP method (e.g. 'GET', 'POST', 'PUT', ...) should be specified explicitly as an argument to the makeRequest function.
• If we want to generate an HTTP payload based on a specific schema, this should be done using a separate function (outside of the makeRequest function) and the output of that function should then be passed explicitly as parameters to the makeRequest function.
• Also, if we remove all the internal swagger-specific logic from the makeRequest function, we should probably move the makeRequest function to a different file.

Actual behavior

There is complicated internal logic which transforms input parameters to match specific swagger schemas - This makes it hard to understand what the resulting HTTP request looks like; it's often misleading because the parameters argument that you pass to makeRequest doesn't necessarily look like the payload/query which will be sent in the HTTP request.

https://github.com/LiskHQ/lisk/blob/adaad370c81b7a205c18377ee7b55dba55f730a9/test/common/swagger_spec.js#L153-L176

Steps to reproduce

Look at existing HTTP functional tests.

[diego-G]

I totally agree on this. Basically we are transforming the data to match our Swagger specs and "miraculously" passing the tests.

[nazarhussain]

@jondubois @diego-G The concept behind swagger_spec contains two important facts:

1. Create a request object once and then send it to server multiple times
2. Utilize the information available in swagger.yml as much possible

The point one is important because we are doing it across the tests undoubtably infinite times. We create a request object and then send it with different parameters for different test cases. When you create a request object you specify it GET, POST, PUT once, so why you don't need to repeat it ever time you send the request with makeRequest

The second point is utilized to make easy when you test an API which is defined extensively with swagger.yml file. So you don't need to remember/specify all the time which parameter to be send as body and which to be send as query param. Because that's all defined very well in swagger.yml file. You just specify the name of the param and swagger_spec will aware which type of param it is. We are not mutating any of the param, we are just specifying them on the right place.

e.g. Making a request and ending it to server is that simple.

const delegatesEndpoint = new swaggerEndpoint('GET /delegates');

delegatesEndpoint.makeReuest({limit: 10})...;
delegatesEndpoint.makeReuest({offset: 2, limit: 10})...;
delegatesEndpoint.makeReuest({publicKey: ''})...;

Look how easy it is to make a request once and send it multiple times with different parameters. And that's what intention of the test cases is. The swagger_spec was designed to specifically utilized in test cases. Its not a library to be utilized by developers outside the test suite.

Now the last question, what the makeRequest is doing after request is done. Is pretty straight forward.

expect(res.statusCode).to.be.eql(expectedResponseCode);
expect(res.headers['content-type']).to.match(/json/);
expect(res.body).to.be.validResponse(
    self.getResponseSpecPath(expectedResponseCode)
);

It validate the request status code as you defined or as defined as default response in swagger.yml. It validates the response schema appropriately. This helps us to validate all request/response everywhere in the test suite. We want to avoid that repetitive code in every test case.

Hope that answered to your questions, let me know if I can explain it further.

[jondubois]

@nazarhussain in retrospect, I think that point 1 is not very important and the current approach of specifying the HTTP method and route once is probably fine.

I'm more worried about the second point highlighted by @diego-G's comment. The parameters are not being sent directly as the HTTP request payload/query as expected; instead they are being reformatted based on swagger definitions.

[shuse2]

I agree with @jondubois and @diego-G
the two suggestions can be done pretty easy with the default features
for first point, creating the server can be done in beforeEach block in each tests
and second point, can be done in simple util function.

I think the point of the swagger is to provide the updated docs with api implementation.
In my opinion, reading the docs from the test code seems over simplifying the process for test, and losing the purpose of the test.

[nazarhussain]

@shuse2 Consider swagger_spec as the the utility function which already exists so no need to create new. Swagger in our case not just providing documentation. It has a tight integration in the implementation of the REST APIs.

To write tests for APIs, one need to aware of supported parameters and other details, so reading that information on a HTML page or reading it from YML file is the same thing. You can't write tests for APIs unless you are aware of the detailed specs of it. And swagger_spec is just helping you out in that process and removing a lot of boilerplate duplicate code.

Think of one simple example.

1. You need to send search param to endpoint /delegates
2. You will go and look into documentation, does that search param is provided as query-string or url-encoded form
3. Then you will write specific code to facilitate either case
4. Then you will send the request using supertest

You have to repeat these 4 steps for every test scenario, and that will create a lot of redundent code in your test file. SwaggerSpec is a utility class that helps you to perform above 4 steps with an easy interface, so you can actually focus on actual expectations in your tests.

[jondubois]

I think that test logic should be as simple as possible. Trying to make it too convenient or flexible can lead to bugs in the test code.

For example, now to POST a signature, I need to do something like this:

var signatureObject = apiHelpers.createSignatureObject(
  scenario.multiSigTransaction,
  scenario.members[0]
);
signatureEndpoint.makeRequest({ signature: signatureObject }, 200)

So reading the test, it looks like the payload/body for the POST request will be this:

{
  signature: {
    "transactionId":"4098698890560455004",
    "publicKey":"4bec05bd550f4203e1110c85a2be062549e056ead85d75330882fd7db6d340fe",
    "signature":"1e1296ea34a2d66ed7a3f57740818769dfd5658..."
  }
}

But in reality, the signature field gets automatically removed and only this is sent instead:

{
  "transactionId":"4098698890560455004",
  "publicKey":"4bec05bd550f4203e1110c85a2be062549e056ead85d75330882fd7db6d340fe",
  "signature":"1e1296ea34a2d66ed7a3f57740818769dfd5658..."
}

So that's correct (it's what I wanted functionally) but I found that confusing and I had to read through all the makeRequest logic and also swagger.yml to understand what that simple test case was doing. It could confuse new users.

[nazarhussain]

@jondubois That's because of not understanding of swagger concepts and conventions. In swagger request body must be labeled as name. So in your case for that particular endpoint request body was labeled as signature, and we use same concept while receiving the params

context.request.swagger.params.signature.value

While processing request we didn't say req.body, we get param with a name. So even if you don't look into spec and just look at the implementation of the endpoint there will be no confusion. You passed a param signature to makeRequest that param will be accessible by request.swagger.params.signature.

As I said earlier we are not using swagger just for API documentation, its tightly integrated into implementation. So you have to test accordingly to align test and implementation.

[shuse2]

I think one of the benefits to write test is to show what the code actually do.
Also, I think we shouldn't trust the swagger definition in the test, and it shouldn't matter how the parameter is used in the source code.

as @jondubois described,
the contents on the body is not really obvious, which loses readability of the test.

[nazarhussain]

@shuse2 I believe readability comes with knowledge. If you read js-doc for makeRequest you will know object passed to it will be object of parameters not the body of the request. If you feel the description there is not sufficiently described, please update it. And while calling makeRequest one must read its doc to see what it is accepting as parameters.

[jondubois]

@nazarhussain It sounds like we should all have a more in-depth discussion about this.

[shuse2]

I have come across the issue regards to this.
If you send transaction like below from test suite, it approves by changing the recipientId to empty string. However, if you try it from actual post, it fails.

{  
   "amount":"0",
   "recipientId":null,
   "senderPublicKey":"7898fda1acce6fc870d152d4b3a06efde6dfe90059177d63790c9e09e4fd313b",
   "timestamp":57430473,
   "type":2,
   "fee":"2500000000",
   "asset":{  
      "delegate":{  
         "username":"shuse2"
      }
   },
   "signature":"f0f020aecf429ad42ac1d52d7b738ec0b8f9391e915011de33cba84e3ec78b6c3ae56d2a7642dfae8f3ca250add2eaf0069518749cf89a26dbe2a164f5116a01",
   "id":"1233506970768755779"
}

[nazarhussain]

@shuse2 That's a check in test suit, should be removed, waited for Lisk-Elements to be updated. When it will be updated, we will not transform any transaction attribute in the test-suit. And Lisk Elements will have to generate transaction in the format Swagger documented. So "recipientId" should be string, while "null" is an object, that what cause the validation to fail.

[diego-G]

This commit https://github.com/LiskHQ/lisk/commit/8b6e100f38169e20dbb3987b166ee3a50f898d49 align with LiskHQ/lisk-js#605 make our test suites more reliable and easier to read. Hope @jondubois and @shuse2 agree.