proposal for a simpler check() API

[sniku]

The current check() API seems to be unnecessarily complex and lengthy.

Consider an example:

let r = http.post(`https://example.com/auth/login`, { username: 'user', password: 'pass!'});

check(r, {
    "login successful": (r) => r.status === 200,
}) || fail("could not log in");

It seems unnecessary to require an object of arrow functions to make a simple check.

A simpler solution would be to write it this way:

let r = http.post(`https://example.com/auth/login`, { username: 'user', password: 'pass!'});

check(r.status === 200, "login successful") || fail("could not log in");

Even for batch checks, this format is less readable for me. Example:

check(res, {
     "status is 200": (r) => r.status === 200,
     "body is 1176 bytes": (r) => r.body.length === 1176,
     "is welcome header present": (r) => r.body.indexOf("Welcome!") !== -1
});

and alternative:

check(r.status === 200, "status is 200");
check(r.body.length === 1176, "body is 1176 bytes");
check(r.body.indexOf("Welcome!") !== -1, "is welcome header present");

IMO multiple checks are more readable, than an object of arrow functions.

[ppcano]

I think allowing grouping multiple checks is a nice API for some cases.

     let defaultReqChecks = {
         "status is 200": (r) => r.status === 200,
         "duration under 150ms": (r) => r.timings.duration <= 150
         "duration under 200ms": (r) => r.timings.duration <= 200
     }
     .....
     let r1 = http.post(`https://example.com/auth/login`, { username: 'user', password: 'pass!'});
     check(r1, defaultReqChecks);
     .....
      let r2 = http.get(`https://example.com/account/items`);
     check(r2, defaultReqChecks);

But I think the current API could not be very intuitive for newcomers. check is one of the most used APIs in k6 and I'd love to include this suggestion to make it more versatile for other writing styles.

For me, check is very similar than an assertion in other testing frameworks, I would like to do something like:

    check(r.status === 200, "status is 200");
    check(r.body.length === 1176, "body is 1176 bytes");
   

   //  even without check title
    check(r.status === 200);

   
   if (check(r.status === 200 && res.data)) {
          // check the data here
   }

I would like k6 to support both options.

[na--]

I agree that the current check() syntax is unnecessarily verbose and over-complicated... The problem is that we can't really touch it, since it would majorly break backwards compatibility - check() is used in tons of user scripts and official k6 examples... But while we can't change it, we can easily add a new function with the API you propose, we just have to figure out an appropriate name 😄 Potential ideas: assert() (most liked so far), verify(), test() (:-1:), ... suggestions?

I personally prefer assert(), since it would be familiar to users... Also, what we intend to use it for is somewhat similar to what assertions are normally used for AFAIK (hopefully any differences, if they exist, won't confuse people).

Edit:

For me, check is very similar than an assertion in other testing frameworks, I would like to do something like:

Glad to see we're on the same page 👍 😀

[na--]

To take the assertion similarities one step further, it seems to me that there might be a very valid use case for assert() (or another new function) to abort the iteration early if the assertion fails. For example, this would be very useful when some script logic or HTTP request depends on the response to a previous request. This will also obviate the need for manually aborting the script via something like throw new Error() or the objectionable assert() || fail("some reason").

Actually, AFAIK, I think that "assert"-like constructs frequently abort the program execution when their condition isn't satisfied, but not always. For example, the Go testify library we use in k6 has two main modules: assert and require. Both have the same helper check functions, but the assert functions only mark the test as failed, without aborting its execution, while the require ones fail the test AND abort its execution. Unfortunately, require has completely different connotations in JavaScript land, so we can't reuse that. I'm open to suggestions.

I'm not sure what the best UX here is... Should assert() failures abort the iteration or not? I definitely think both behaviors have merits and deserve their own helper functions, I'm just not sure what the assert() behavior should be and what the other function should be called... check() is already taken, reuire() can't be used, so any thoughts and ideas?

[na--]

Adding a note here for a minor technical detail that I don't think will be an issue, but it would be best if we benchmark, just in case... We'd be switching from a single check() function call with X multiple checks to X assert() calls, one per check condition. Depending on the average value of X, this could add many more context switches between the goja JS execution and our Go code. Checks already have context switches because of the lambdas, so we need to benchmark that anyway.

[ppcano]

I think we are discussing a few related issues and each one might deserve its own issue.

1 - Should the check API provide an additional syntax to use it?

For this type of API changes that cannot be iterated without introducing breaking change, we have discussed that an option could be to provide different module/namespaces for the new API. From example:

      import check from `k6/check`
         or
      import { check } from `k6/latest`

This could allow showing deprecation messages for the previous API until the new API becomes the standard.

2 - Should the check API provide an option to exit the iteration if the condition fails?

to abort the iteration early if the assertion fails
Should assert() failures abort the iteration or not?

I think there is also a need of having an API to exit the iteration and the fail API may not solve this properly.

It's fine for me if check/assert has the option to exit the iteration but I'd like to have also an independent API; something like:

    
      if (!res.status === 200) {
        exit();
     }

     // or

      if (check(res.status === 200)) {
         ...
     } else {
        exit();
     }

3 - The conversion brought up the question if check is the correct name for this API and if it is convenient to rename it to assert/verify.

[na--]

1 - Should the check API provide an additional syntax to use it?

I think that would be needlessly very confusing...

This could allow showing deprecation messages for the previous API until the new API becomes the standard.

As you've mentioned above, the current check() function would still be useful (and I'd say better) in some specific use cases (many conditions or same conditions in many checks), so I see no need to deprecate it.

It's fine for me if check/assert has the option to exit the iteration but I'd like to have also an independent API;

But we already have that - fail() and throw new Error() do that job splendidly...

3 - The conversion brought up the question if check is the correct name for this API and if it is convenient to rename it to assert/verify.

But check is already the name of an existing API that we won't be able to touch for a very long time -if we even want to, since as I mentioned above, I don't... I was not trying to rename anything, rather I was suggesting fitting alternative names to the new API you've proposed.

[sniku]

I think the batch checks are unnecessarily confusing when the batch uses more than one response.

See example. (note the first argument to check() )

const delRes = http.del("https://test-api.loadimpact.com/my/crocodiles/23/");
const getRes = http.get("https://test-api.loadimpact.com/my/crocodiles/23/");

const isSuccessfulDelete = check(null, {
  'croc was deleted': () => delRes.status === 204,
  "deleted croc can't be retrieved": () => getRes.status === 404,
});

I needed to use two different variables in this batch: delRes and getRes. For that reason, I couldn't use the first argument to check().
One could argue that in this case, I should have used two checks, but I dislike this idea for two reasons:

1. My code would be longer.
2. These two checks are related, so my intuition is to batch them.

The proposed new check() function would not have this issue.

[na--]

I agree, and I fully support a new simpler function, we just have to agree on a name and semantics for it 😉 ...

And why I see how your particular example can be a bit annoying, if it was a real http.batch() (you mention batch, but use separate requests 😕 ), you could just pass its result to check() and verify its elements, like the example in https://github.com/loadimpact/k6#checks-and-thresholds does... Far from ideal, especially given #767, but no need to pass null.

Or in your case, as you didn't use a batch, you could have passed [delRes, getRes] or {get: getRes, del: delRes} or just assigned the responses straight in an object that you pass to check.

[simskij]

If we want to align with popular js testing frameworks, we could consider going for expect. However, this usually implements a fluent interface for chaining function calls, which might be overkill for our use case:

expect(someExpression).toEqual(someValue);
expect(someExpression).toBeTruthy();
expect(someInt).toBeGreaterThan(3);
expect(somethingVoid).not.toBeDefined();

Adding labels to this could be done using another function chain, like:

expect(something).toBe(something).applyLabel('some string');

// or, as is common in js testing frameworks:

it('should something', () => {
  expect(something).toBe(something);
  expect(somethingElse).toBe(somethingElse);
});

[oleiade]

The team discussed this during our backlog review, and we decided to close this issue in favor of a newer one once we have had the opportunity to reevaluate the product aspects of the checks API. Feel free to reopen this down the road if you stumble upon this and with have failed to do so ☝🏻