Update use_role testing in test_run_processor #1477
Conversation
| # Test create_dev_app with no existing application AND create succeeds AND app role == package role | ||
| @mock.patch(RUN_PROCESSOR_GET_EXISTING_APP_INFO, return_value=None) | ||
| @mock.patch(NATIVEAPP_MANAGER_EXECUTE) | ||
| @mock_connection() | ||
| def test_create_dev_app_w_warehouse_access_exception( | ||
| mock_conn, mock_execute, temp_dir, mock_cursor | ||
| def test_use_role_in_create_dev_app_w_no_additional_privileges( | ||
| mock_execute, mock_get_existing_app_info, temp_dir, mock_cursor |
There was a problem hiding this comment.
this test_use_role_in_create_dev_app_w_no_additional_privileges and the below test_use_role_in_create_dev_app_w_additional_privileges are the two that test use_role flows in create_or_upgrade_app.
| @mock.patch(NATIVEAPP_MANAGER_EXECUTE) | ||
| @mock.patch(NATIVEAPP_MANAGER_USE_ROLE) | ||
| @mock_connection() | ||
| def test_create_dev_app_w_warehouse_access_exception( | ||
| mock_conn, mock_use_role, mock_execute, temp_dir, mock_cursor | ||
| ): |
There was a problem hiding this comment.
In this test and the one below, we mock use_role with@mock.patch(NATIVEAPP_MANAGER_USE_ROLE). The mocking of _execute_query that was made as part of use_role are removed from mock_execute_helper.
| ( | ||
| mock_cursor([{"CURRENT_ROLE()": "old_role"}], []), | ||
| mock.call("select current_role()", cursor_class=DictCursor), | ||
| ), | ||
| (None, mock.call("use role package_role")), |
There was a problem hiding this comment.
hmm, but not executing "use role" at the right time means that the behaviour could be incorrect. Removing boilerplate is good, but aren't we making the test weaker as a result?
There was a problem hiding this comment.
the idea is we only need to test the behavior of use_role once for each occurrence of it. So if we are writing tests for one function (that uses use_role) we can write one (or as many needed) test to make sure that use_role works as expected. The rest of the tests we write for this function can mock use_role.
There was a problem hiding this comment.
In this example, use_role behavior is tested in test_get_app_pkg_distribution_in_snowflake. The rest of the tests that are written for get_app_pkg_distribution_in_snowflake in particular, can mock use_role.
There was a problem hiding this comment.
For a happy path vs exception path pair of tests, that might make sense in some cases, but I'm not sure I'm generally aligned with this idea. And even then, it seems like a micro-optimization to me. As Cam was saying earlier today, DRY isn't necessary a target in tests, and often will end up with negative side effects. Happy to discuss more when I'm back if you want.
|
Closing this in favor of a proper longer-term solution. |
Pre-review checklist
Changes description
This is the first (small) step toward improvements to unit tests. Epic link
A WIP document of areas of improvement I've found so far [WIP] Current issues found in SnowCli tests
Example to show how we can update testing of use_role, so we can write less boilerplate. Using run_processor testing as an example.
...