Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

should match each fail if the array is empty #2364

Closed
ptrthomas opened this issue Jul 20, 2023 · 10 comments
Closed

should match each fail if the array is empty #2364

ptrthomas opened this issue Jul 20, 2023 · 10 comments
Assignees
Milestone

Comments

@ptrthomas
Copy link
Member

this has come up a few times and new users run into this a little too often for comfort

I think it is fair that the intent is always to validate more than one item in the array.

example: https://stackoverflow.com/q/60638091/143475

@adrian-85
Copy link

So match each does not perform path validation, only content, if it exists. Which means if the user is uncertain of their https response, they will need to do some schema validation first.

I actually was not aware of this either. But from a best practice perspective, we should start out with the most important assertions first, such as expected data existing, before moving onto validating the content of that data. Layers of assertions can seem painful, but when it comes to debugging, it can be really helpful to break down the response in this way.

Although we do have all-in-one's like contains only so I can see a "spoiled" community wanting the same here.

@ptrthomas ptrthomas self-assigned this Aug 9, 2023
@ptrthomas ptrthomas added the fixed label Aug 9, 2023
@ptrthomas
Copy link
Member Author

ptrthomas commented Aug 9, 2023

this is implemented, I am a bit worried if teams extensively using nested schemas will have issues (see picture below). in which case we may need a "lenient" match each only for the short-cut. for example: #[?] foo

image

actual test:

Scenario: complex nested arrays
* def json =
"""
{
  "foo": {
    "bars": [
      { "barOne": "dc", "barTwos": [{ title: 'blah' }] },
      { "barOne": "dc", "barTwos": [{ title: 'blah' }], barThrees: [{ num: 1 }] }
    ]
  }
}
"""
* def barTwo = { title: '#string' }
* def barThree = { num: '#number' }
* def bar = { barOne: '#string', barTwos: '#[] barTwo', barThrees: '##[] barThree' }
* match json.foo.bars == '#[] bar'

@ptrthomas ptrthomas added this to the 1.4.1 milestone Aug 9, 2023
@steve1337
Copy link

steve1337 commented Aug 24, 2023

I am a bit worried if teams extensively using nested schemas will have issues

@ptrthomas Just providing some feedback: This is indeed problematic for me.

I just had a CI fail because I didn't pin the karate version (on purpose I think we should use latest).
As a workaround I pinned the version to 1.4.1.RC2 for the time being.

13:44:38.275 [main] ERROR com.intuit.karate - test/integration/tests/notification.util.feature:11
And match each response.docs == call read('../schemas/notification.schema.js')
match failed: EACH_EQUALS
  $ | match each failed, empty array / list (LIST:MAP)
  []
  {"_id":"#string",...}

This (and other tests) started failing w/ 1.4.1.RC3.

How do I need to update our tests / schemas to prevent test failures for empty arrays?

@ptrthomas
Copy link
Member Author

@steve1337 in at least this case, since $ = [] maybe you should skip this test or just * match response.docs == []

let me know if you run into more issues, we are open to un-doing this change

@steve1337
Copy link

steve1337 commented Aug 24, 2023

@ptrthomas Thanks for the fast answer.

I want to provide and explain my use case / impl a bit more because unfortunately this won't work in this case

I have a lot of tests like this

Feature: Resource CRUD

    Scenario: Create multiple resources and verify notification are created correctly

        And def data = call read('notification.util.feature@getAll')
        Then def docs = data.response.docs
        And match docs == `#[0]`
        And def initialNotificationCount = docs.length

        Given url `${API_URL}/resource`
        And request ({ key: "val" })
        When method POST
        Then status 201

        And def data = call read('notification.util.feature@getAll')
        And def docs = data.response.docs
        And match docs == `#[${initialNotificationCount + 1}]`

        Given url `${API_URL}/resource`
        And request ({ key: "val" })
        When method POST
        Then status 201
        
        And def data = call read('notification.util.feature@getAll')
        And def docs = data.response.docs
        And match docs == `#[${initialNotificationCount + 2}]`

Then my util feature like this notification.util.feature

@ignore @report=false
Feature: Notification UTILS

@getAll
Scenario: Get ALL notifications for <page> (default = 1)
    * def activityPage = karate.get('page', 1)
    Given url `${API_URL}/activity?page=${activityPage}&sortBy=desc(created)`
    When method GET
    Then status 200
    And match response == call read('../schemas/paginatedResponse.schema.js')
    And match each response.docs == call read('../schemas/notification.schema.js')

I re-use this pattern in a bunch of places as well.

How would you suggest I adjust the code to fit the new changes? I'm not really sure how I would do that because I def want to check the schema once the array is not empty.

@adrian-85
Copy link

@steve1337 One thing you could do is set object to be optional ##(object), then define the object variable as null before performing your first assertion when length is zero. Then set it again for your schema validation, and the rest of the assertions should pass.

@ptrthomas
Copy link
Member Author

all: I've checked in what I think is a reasonable solution for backwards compatibility. those who have extensive use of match each or #[] shortcuts AND where you have some of those run against empty arrays, you can just add * configure matchEachEmptyAllowed = true and get the old behavior where needed

image

@ptrthomas
Copy link
Member Author

adding a comment for future consideration. I think the ideal solution would be to support this syntax:

* match each? actual == expected
* match actual == '#[?] expected'

this was assessed to be too complex to implement at this point. there's a TODO comment in the code for MatchOperation:

Match.Type is currently an enum and should ideally be a composite object. so we have an explosion of enum values to represent all permutations of core type, deep, each and now this "optional each" concept

@ptrthomas
Copy link
Member Author

@steve1337 1.4.1.RC4 is available with the change I described two comments above, so you can add * configure matchEachEmptyAllowed = true only where you want the old behavior, would be great to get your feedback before releasing final

@ptrthomas
Copy link
Member Author

1.4.1 released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants