Copilot commented on code in PR #42411: URL: https://github.com/apache/superset/pull/42411#discussion_r3659567840
########## tests/unit_tests/commands/databases/exceptions_test.py: ########## @@ -0,0 +1,42 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from superset.commands.database.exceptions import ( + DatabaseExtraJSONValidationError, + DatabaseExtraValidationError, +) + + +def test_database_extra_validation_error_interpolates_key() -> None: + """ + The message is built with a lazy string, so a malformed placeholder only + blows up when the error is rendered (e.g. serialized into an API response). + Force that rendering here. + """ + error = DatabaseExtraValidationError("metadata_params") + message = str(error.messages[0]) + + assert "metadata_params" in message + assert "%" not in message + + +def test_database_extra_json_validation_error_interpolates_json_error() -> None: + error = DatabaseExtraJSONValidationError("Expecting value: line 1 column 1") + message = str(error.messages[0]) Review Comment: `ValidationError` is instantiated with `field_name=\"extra\"` in both exception classes, which in marshmallow typically makes the stored messages structured by field (e.g. a dict keyed by the field name). Indexing `error.messages[0]` is likely to fail (TypeError/KeyError) or not assert against the intended rendered message. Prefer asserting against the message under the `extra` field (or via the API marshmallow uses to normalize messages) so the test matches how the error is serialized. ########## tests/unit_tests/commands/databases/exceptions_test.py: ########## @@ -0,0 +1,42 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from superset.commands.database.exceptions import ( + DatabaseExtraJSONValidationError, + DatabaseExtraValidationError, +) + + +def test_database_extra_validation_error_interpolates_key() -> None: + """ + The message is built with a lazy string, so a malformed placeholder only + blows up when the error is rendered (e.g. serialized into an API response). + Force that rendering here. + """ + error = DatabaseExtraValidationError("metadata_params") + message = str(error.messages[0]) Review Comment: `ValidationError` is instantiated with `field_name=\"extra\"` in both exception classes, which in marshmallow typically makes the stored messages structured by field (e.g. a dict keyed by the field name). Indexing `error.messages[0]` is likely to fail (TypeError/KeyError) or not assert against the intended rendered message. Prefer asserting against the message under the `extra` field (or via the API marshmallow uses to normalize messages) so the test matches how the error is serialized. ########## tests/unit_tests/commands/databases/exceptions_test.py: ########## @@ -0,0 +1,42 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from superset.commands.database.exceptions import ( + DatabaseExtraJSONValidationError, + DatabaseExtraValidationError, +) + + +def test_database_extra_validation_error_interpolates_key() -> None: + """ + The message is built with a lazy string, so a malformed placeholder only + blows up when the error is rendered (e.g. serialized into an API response). + Force that rendering here. + """ + error = DatabaseExtraValidationError("metadata_params") + message = str(error.messages[0]) + + assert "metadata_params" in message + assert "%" not in message Review Comment: Asserting that no percent sign appears anywhere in the rendered message is broader than necessary and can make the test brittle (e.g., if translations or future wording include a literal `%`). To specifically catch interpolation/placeholder issues, assert that formatting patterns remain absent (e.g., no `%(...)` / `%{...}` sequences) rather than banning `%` entirely. ########## tests/unit_tests/commands/databases/exceptions_test.py: ########## @@ -0,0 +1,42 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from superset.commands.database.exceptions import ( + DatabaseExtraJSONValidationError, + DatabaseExtraValidationError, +) + + +def test_database_extra_validation_error_interpolates_key() -> None: + """ + The message is built with a lazy string, so a malformed placeholder only + blows up when the error is rendered (e.g. serialized into an API response). + Force that rendering here. + """ + error = DatabaseExtraValidationError("metadata_params") + message = str(error.messages[0]) + + assert "metadata_params" in message + assert "%" not in message + + +def test_database_extra_json_validation_error_interpolates_json_error() -> None: + error = DatabaseExtraJSONValidationError("Expecting value: line 1 column 1") + message = str(error.messages[0]) + + assert "Expecting value: line 1 column 1" in message + assert "%" not in message Review Comment: Asserting that no percent sign appears anywhere in the rendered message is broader than necessary and can make the test brittle (e.g., if translations or future wording include a literal `%`). To specifically catch interpolation/placeholder issues, assert that formatting patterns remain absent (e.g., no `%(...)` / `%{...}` sequences) rather than banning `%` entirely. -- 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]
