No error raised when using GQLRequestMutation type

[shahzadlone]

There are instances where not all our Mutation Types behave the same.

For example these two tests for GQL Mutation show the behavior where GQL does not return/raise an error: TestACP_CreateWithIdentityAndUpdateWithoutIdentityGQL_CanNotUpdate TestACP_CreateWithIdentityAndUpdateWithWrongIdentityGQL_CanNotUpdate

But these two tests for other MutationTypes do return an error: TestACP_CreateWithIdentityAndUpdateWithoutIdentity_CanNotUpdate TestACP_CreateWithIdentityAndUpdateWithWrongIdentity_CanNotUpdate

When resolving this, make sure all error types that are returned from each mutation type are consistent.

Also likely can add another prop for explicit use of DocID for the UpdateDoc action instead of only allowing the use of DocID which is an index (so we can test invalid cases). This would catch this case of inconsistent error for other more common places.

Maybe also try doing a ripgrep or grep search like: rg "SupportedMutationTypes" to see tests that operate on subset of MutationTypes but not all (due to inconsistent behavior).

[AndrewSisley]

which will pass the test

Please do not make the test pass, instead skip it.

[AndrewSisley]

Although, come to think about it I dislike this NoGQLError boolean. We already had/have a means for handling this - the TestCase can have a property that says it only supports certain mutation types. If it doesn't exist at the moment, you can find the old code in the git history and use that maybe, or just rewrite it.

The test should not look valid (and passing) and create a false sense of security.

EDIT: I checked, this still exists - see TestCase.SupportedMutationTypes, and just use that wherever you were thinking of using the boolean.

[shahzadlone]

Please do not make the test pass, instead skip it.

Why should we skip it, it needs to be asserted as it is asserting a correct behavior. It's just that the correct behavior in GQL case does not output a visible error.

Although, come to think about it I dislike this NoGQLError boolean. We already had/have a means for handling this - the TestCase can have a property that says it only supports certain mutation types. If it doesn't exist at the moment, you can find the old code in the git history and use that maybe, or just rewrite it.

The test should not look valid (and passing) and create a false sense of security.

EDIT: I checked, this still exists - see TestCase.SupportedMutationTypes, and just use that wherever you were thinking of using the boolean.

The test should still pass and assert even for GQL client, it's not a false sense of security, just that an extra error is not popping up, the behavior should still be asserted and caught if incase it's buggy. One additional thing I can do in the:

if action.NoGQLError && mutationType == GQLRequestMutationType && !expectedErrorRaised {
    return  
}

is to add an other condition where this only happens for that specific error string that we want to still pass for even if that error sting is not raised.

[AndrewSisley]

Lets talk about this in the standup. I very much want all our clients/mutation types to work in the same way, it makes things a lot easier for us and the users IMO. If we want them to behave the same way, then this missing error is a bug, if not, I still think a highly output specific NoGQLError prop is a poor solution to the problem you are trying to solve.

[shahzadlone]

I very much want all our clients/mutation types to work in the same way

Same here, however, that seems to be an existing bug, GQL request on a document that does not exist will not return an error.

Note: I am not talking about ACP here, just normal non-ACP state.

I am guessing the planner is the one that suppresses the error.

In contrast, I am fairly confident that the other (non-GQL) mutation types will return an error. However sadly there are no tests asserting the invalid doc ID case as the UpdateDoc test action using an index (which enforces only testing valid docIDs).

[AndrewSisley]

If it is a bug, the test should not pass, and it very definitely should not look like it passes. This use case is exactly why TestCase.SupportedMutationTypes exists.

[shahzadlone]

If it is a bug, the test should not pass, and it very definitely should not look like it passes. This use case is exactly why TestCase.SupportedMutationTypes exists.

I am not talking about my ACP test in my previous response. I am talking about the current state of non-acp behaviour.

[shahzadlone]

The action it self does not perform the mutation (which is correct behavior even for GQL request).

And the test action before and after that action assert the correct behavior is happening, if we skip all of these assertions will skip too.

The only incorrect thing in that test is that for GQL the error doesn't explicitly raise.

[AndrewSisley]

I am failing to see the problem.

We have some test actions to perform. Some mutation types result in an error, some do not. Write two sets of tests using TestCase.SupportedMutationTypes, one expecting the error, and one set without the error.

[shahzadlone]

I am failing to see the problem.

We have some test actions to perform. Some mutation types result in an error, some do not. Write two sets of tests using TestCase.SupportedMutationTypes, one expecting the error, and one set without the error.

That suggestion makes sense. I thought you were saying to skip that test completely haha. I am happy with that route.

[shahzadlone]

Do we want to keep this ticket open for making our clients eventually behave the same in future?

[AndrewSisley]

Do we want to keep this ticket open for making our clients eventually behave the same in future?

Yes please, and please tweak the description, and link to this item via comments above any relevant tests that you are writing (e.g. below), and also note the existence of those tests in this issue's description (if there are a lot, just suggest a Ctrl+f for the link or something).

// This test documents unwanted behaviour, for details please see:
// https://github.com/sourcenetwork/defradb/issues/2410
func TestFoo(t *testing.T) {