A bug in testing framework if error is raised and assertion of the error passes, it omits other assertions

[shahzadlone]

Describe the problem Our testing framework seems to be not asserting the other result values once an error is encountered and assertion for that ExpectedError passes.

To Reproduce Both the below tests pass (Results field in each is different):

func TestStopsAssertionOnceRaisedErrorIsAsserted1(t *testing.T) {
    test := testUtils.RequestTestCase{
        Description:   "Delete multiple documents that exist sub selection, should give error.",
        Request:       `mutation { delete_user(ids: ["bae-xyz"]) }`,
        Results:       []map[string]any{{"_key": "Doesn't assert the result if raised expected error."}},
        ExpectedError: "Field \"delete_user\" of type \"[user]\" must have a sub selection.",
    }
    executeTestCase(t, test)
}

func TestStopsAssertionOnceRaisedErrorIsAsserted1(t *testing.T) {
    test := testUtils.RequestTestCase{
        Description:   "Delete multiple documents that exist sub selection, should give error.",
        Request:       `mutation { delete_user(ids: ["bae-xyz"]) }`,
        Results:       []map[string]any{},
        ExpectedError: "Field \"delete_user\" of type \"[user]\" must have a sub selection.",
    }
    executeTestCase(t, test)
}

Expected behavior The test should assert other fields (like Results in this case) in this case, and ensure tests only pass if all testing args are what we expect them to be, which doesn't make sense so preferably not allow this at all in the testing framework.

[AndrewSisley]

I do see this as by design, any the results of any action returning an error should not be trusted and should instead be ignored.

The example test cases above use the old system, which only allows one expected error scoped to the entire test case. The new system scopes the error to a specific request, allowing one to error, and subsequent actions to still be handled correctly. IIRC a few of our tests rely on this.

Suggest we close this issue.

[shahzadlone]

I do see this as by design, any the results of any action returning an error should not be trusted and should instead be ignored.

The example test cases above use the old system, which only allows one expected error scoped to the entire test case. The new system scopes the error to a specific request, allowing one to error, and subsequent actions to still be handled correctly. IIRC a few of our tests rely on this.

Suggest we close this issue.

I am not sure I understand this fully, so I tested this on one of the new tests and the tests still pass if the last action has a matching error but incorrect result. It passed with or without Results: []map[string]any{{"incorrect": "Bad Result."}},.

func TestSchemaUpdatesTestAddFieldBlockedByTest(t *testing.T) {
    test := testUtils.TestCase{
        Description: "Test schema update, failing test blocks new field",
        Actions: []any{
            testUtils.SchemaUpdate{
                Schema: `
                    type Users {
                        Name: String
                    }
                `,
            },
            testUtils.SchemaPatch{
                Patch: `
                    [
                        { "op": "test", "path": "/Users/Schema/Name", "value": "Author" },
                        { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "Email", "Kind": 11} }
                    ]
                `,
                ExpectedError: "test failed",
            },
            testUtils.Request{
                Request: `query {
                    Users {
                        Name
                        Email
                    }
                }`,
                ExpectedError: "Cannot query field \"Email\" on type \"Users\"",
                Results:       []map[string]any{{"incorrect": "Bad Result."}},
            },
        },
    }
    testUtils.ExecuteTestCase(t, []string{"Users"}, test)
}

[AndrewSisley]

I am not sure I understand this fully, so I tested this on one of the new tests and the tests still pass if the last action has a matching error but incorrect result. It passed with or without

Yes, but in that case you are expecting both a result and an error from a single action, which makes no sense IMO. If you want to test something after an error, use another action.

[shahzadlone]

Yes, but in that case you are expecting both a result and an error from a single action, which makes no sense IMO. If you want to test something after an error, use another action.

Seems like we are saying the same thing. It makes no sense to allow inputting the expected result together with the expected error in the same action. i.e. the testing framework should return an error instead of asserting only half of the action args and passing like it does now.

[AndrewSisley]

Yes, but in that case you are expecting both a result and an error from a single action, which makes no sense IMO. If you want to test something after an error, use another action.

Seems like we are saying the same thing. It makes no sense to allow inputting the expected result together with the expected error in the same action. i.e. the testing framework should return an error instead of asserting only half of the action args and passing like it does now.

You want to fail the test if both are provided? Could make sense

[shahzadlone]

You want to fail the test if both are provided? Could make sense

Yes exactly. Tests should pass only when the input and output is all as we expect.

[AndrewSisley]

Ah nice - would be safer than silently ignoring it :)