dylanpulver opened a new pull request, #45978: URL: https://github.com/apache/superset/pull/45978
### SUMMARY Six assertions in `commands_tests.py` sat below the call that raises, inside `with pytest.raises(...) as excinfo:`, so none of them ran. They could not have: `excinfo.value` is not bound until the block exits. Each block carried `# noqa: PT012`, which is the rule asking for a single simple statement inside a raises block, so the lint that would have caught this was silenced on every one. Replacing all six expectations with ones nothing can produce leaves the tests passing, which is what shows they were dead. **Two hold once moved out of the block**, so they are moved and now check what they claim: `test_database_tables_list_with_unknown_database` and `test_database_tables_superset_exception`. **Four never held.** `str()` on those exceptions does not return the message the author wrote: | test | expected | what `str(excinfo.value)` actually is | |---|---|---| | `test_connection_db_exception` | "Unexpected error occurred, please check your logs for details" | a `SupersetError` list whose message is the mock's "An error has occurred!" | | `test_connection_db_api_exc` | "Connection failed, please check your connection settings" | a `SupersetError` list | | `test_connection_superset_security_connection` | "Stopped an unsafe database connection" | `test` | | `test_database_tables_exception` | "Unexpected error occurred, please check your logs for details" | `Test Error` | Those four are removed rather than rewritten, because choosing what to assert instead is your call: the user-facing message lives on the error objects rather than on `str(exc)`. If you would like them restored against `excinfo.value.errors[0].message` or similar, say which form you prefer and I will add it. `pytest.raises` still checks the exception type in all four, and `mock_event_logger.assert_called()` is untouched. The `as excinfo` binding goes with the removed lines so `ruff` stays clean. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Not applicable. ### TESTING INSTRUCTIONS ``` pytest tests/integration_tests/databases/commands_tests.py -k "test_connection_db_exception or test_connection_superset_security_connection or test_connection_db_api_exc or test_database_tables_list_with_unknown_database or test_database_tables_superset_exception or test_database_tables_exception" ``` - unchanged: 6 passed - unchanged, with all six expectations swapped for strings nothing produces: 6 passed, so none ran - moving all six out of their blocks: 4 failed, 2 passed, which is how the split above was decided - with this change: 6 passed - with both moved-out expectations broken: 2 failed, so they carry weight `ruff check` is clean on the file. The whole file reports an identical `12 failed, 23 passed, 1 skipped` before and after in my environment, which has no seeded example data; the twelve are import tests unrelated to this change. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
