Copilot commented on code in PR #4795: URL: https://github.com/apache/texera/pull/4795#discussion_r3177505501
########## amber/src/main/python/core/util/thread/test_atomic.py: ########## @@ -0,0 +1,120 @@ +# 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. + +import threading + +import pytest + +from core.util.thread.atomic import AtomicInteger + + +class TestAtomicIntegerSingleThreaded: + def test_default_starts_at_zero(self): + assert AtomicInteger().value == 0 + + def test_initial_value_is_coerced_to_int(self): + # The constructor wraps the input through int(), which lets callers + # pass a numeric string or float and still get a clean integer state. + assert AtomicInteger("7").value == 7 + assert AtomicInteger(3.9).value == 3 # int() truncates toward zero + + def test_inc_returns_new_value_after_adding_default_one(self): + a = AtomicInteger(10) + assert a.inc() == 11 + assert a.value == 11 + + def test_inc_with_custom_delta_uses_int_coercion(self): + a = AtomicInteger(10) + assert a.inc(5) == 15 + # int("3") -> 3, the underlying state increments by 3. + assert a.inc("3") == 18 + + def test_dec_is_inc_with_negated_delta(self): + a = AtomicInteger(10) + assert a.dec() == 9 + assert a.dec(4) == 5 + + def test_get_and_inc_returns_pre_increment_value(self): + a = AtomicInteger(10) + assert a.get_and_inc() == 10 + assert a.value == 11 + + def test_get_and_dec_returns_pre_decrement_value(self): + a = AtomicInteger(10) + assert a.get_and_dec(2) == 10 + assert a.value == 8 + + def test_value_setter_replaces_state_with_int_coercion(self): + a = AtomicInteger(10) + a.value = 42 + assert a.value == 42 + a.value = "100" + assert a.value == 100 + + def test_get_and_set_currently_deadlocks_on_non_reentrant_lock(self): + # Bug pin: get_and_set acquires self._lock and then reads self.value, + # which is a property that ALSO tries to acquire self._lock. The lock + # is a non-reentrant threading.Lock, so the call deadlocks the moment + # it is invoked. Document via thread + timeout so the test surfaces + # the deadlock without hanging the whole suite, and pair it with an + # xfail-strict test below that asserts the intended contract. + a = AtomicInteger(10) + completed = threading.Event() + + def attempt(): + a.get_and_set(99) + completed.set() + + threading.Thread(target=attempt, daemon=True).start() + completed.wait(timeout=0.5) + assert not completed.is_set(), ( + "get_and_set unexpectedly returned — the deadlock bug appears fixed; " + "delete this pinned test along with the xfail below." + ) Review Comment: The deadlock pin can pass even if the worker thread never actually enters `get_and_set` (e.g., scheduling delay) or if `get_and_set` raises an exception before setting `completed`. Consider tracking a `started` event (set at the top of `attempt`) and capturing any exception from `attempt` so the test can assert the thread started, is still alive after the timeout, and didn’t exit due to an error (not just that `completed` wasn’t set). -- 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]
