Use mocked exec_msar_func calls for RPC tests

[Anish9901]

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the develop branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no
  visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[mathemancer]

Good start, but:

You should check the whole call to exec_msar_func, including the connection and function name. This helps these tests catch a whole bunch of easy-to-make errors as we modify the python wrappers.

[mathemancer]

Generally, I'd prefer avoiding using this function in this test. The reason is that it makes it harder to modify the underlying function (e.g., making it so that we don't even need the transform function), since it breaks this test for unrelated reasons.

[Anish9901]

I see your point @mathemancer, but, for the case you're describing I think I would prefer if the test did break, since the way the function under test used to work has changed.

If we were to not use these functions in these tests however, here are some of the possible ways I see to achieve that:

1. Not checking assert call_args[3] == json.dumps(transformed_column_data) (Doesn't seem ideal).
2. Hardcoding the result from the function in the test (Test would still fail if the behavior of the underlying function changes).
3. Using the transformed result as an argument to API call in the test (Test fails since the underlying function expects the argument to be not transformed).

[mathemancer]

I prefer (2).

• It clearly specifies the actual form that needs to be made to the DB layer.
• It verifies that no changes in the transformation have happened unintentionally, for example if someone were to introduce a bug in the _transform_column_alter_dict function itself.
• It detaches the test from the internal implementation of the function being tested as much as possible.

I agree the test should break if we align the API column_data_list with the form of the SQL layer column_data_list, but we don't want it to break because (for example) the function being used in the test is now missing, which it would in that case with this test as currently written. We want it to break because we expect the input and output to be different, but they're now the same. I.e., we want the test assertion to say "Hey! this JSON isn't what I expected", not "Hey! There's a private function missing!".

Hopefully that makes sense.

[mathemancer]

I think you should check all the arguments. That way, if someone fatfingers a function name down the road (i.e., messes up the string describing the SQL function), or accidentally switches the order of the connection and function name, this will catch it. Same for the other similar tests.

Also, we should use something more identifiable than True for the mock connect yield. Maybe something like:

class MockConnection:
    pass

[Anish9901]

I think you should check all the arguments. That way, if someone fatfingers a function name down the road (i.e., messes up the string describing the SQL function), or accidentally switches the order of the connection and function name, this will catch it.

@mathemancer These conditions are already covered by test_db_wrapper. Not only will it check whether the 2nd argument in each exec_msar_func call is a string, it'll also verify that a function specified in the string exists on the underlying database. This would mean that we would also catch any accidentally switches of order between the connection and function name.

[mathemancer]

Ahhhh you're right. I forgot about that.

[mathemancer]

Okay, LGTM!