Copilot commented on code in PR #5657: URL: https://github.com/apache/texera/pull/5657#discussion_r3406831364
########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/scan/AutoClosingIteratorSpec.scala: ########## @@ -0,0 +1,148 @@ +/* + * 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. + */ + +package org.apache.texera.amber.operator.source.scan + +import org.scalatest.flatspec.AnyFlatSpec + +class AutoClosingIteratorSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // hasNext + onClose firing semantics + // --------------------------------------------------------------------------- + + "AutoClosingIterator.hasNext (non-empty underlying)" should + "return true and NOT invoke onClose" in { + var closed = false + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3), () => closed = true) + assert(it.hasNext) + assert(!closed, "onClose must not fire while elements remain") + } + + "AutoClosingIterator.hasNext (exhausted underlying)" should + "return false and invoke onClose exactly once" in { + var closeCount = 0 + val it = new AutoClosingIterator[Int](Iterator.empty, () => closeCount += 1) + assert(!it.hasNext) + assert(closeCount == 1) + } + + it should "NOT invoke onClose again on a second hasNext after exhaustion" in { + var closeCount = 0 + val it = new AutoClosingIterator[Int](Iterator.empty, () => closeCount += 1) + assert(!it.hasNext) + assert(!it.hasNext) + assert(!it.hasNext) + assert(closeCount == 1, s"onClose must fire exactly once, got $closeCount calls") + } + + // --------------------------------------------------------------------------- + // next() — delegates straight through + // --------------------------------------------------------------------------- + + "AutoClosingIterator.next" should "delegate to the wrapped iterator (in order)" in { + val it = new AutoClosingIterator[Int](Iterator(10, 20, 30), () => ()) + assert(it.next() == 10) + assert(it.next() == 20) + assert(it.next() == 30) + } + + // --------------------------------------------------------------------------- + // Full traversal — onClose fires exactly once at the end + // --------------------------------------------------------------------------- + + "AutoClosingIterator full traversal" should + "yield every element of the wrapped iterator in order" in { + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3, 4, 5), () => ()) + assert(it.toList == List(1, 2, 3, 4, 5)) + } + + it should "fire onClose exactly once when toList finishes consuming" in { + var closeCount = 0 + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3), () => closeCount += 1) + val _ = it.toList + assert(closeCount == 1, s"expected single onClose firing, got $closeCount") + } + + // --------------------------------------------------------------------------- + // Already-empty source — close fires on the very first hasNext call + // --------------------------------------------------------------------------- + + "AutoClosingIterator over an already-empty source" should + "fire onClose on the very first hasNext call" in { + var fired = false + val it = new AutoClosingIterator[Int](Iterator.empty, () => fired = true) + val _ = it.hasNext + assert(fired, "onClose must fire when the source is already empty") + } + + // --------------------------------------------------------------------------- + // Mid-iteration close behavior — onClose does NOT fire before exhaustion + // --------------------------------------------------------------------------- + + "AutoClosingIterator (mid-iteration)" should + "leave onClose un-fired between elements (only fires after hasNext returns false)" in { + var fired = false + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3), () => fired = true) + while (it.hasNext) { + it.next() + // After every next(), there is at least one more element OR the + // next hasNext call will fire onClose. Until that point, fired + // must remain false. + // (When the final next() consumes 3, fired is still false because + // the loop hasn't run hasNext again yet.) + } + assert(fired, "after the while loop's final hasNext, onClose must have fired") + } + + // --------------------------------------------------------------------------- + // onClose exception propagation + // --------------------------------------------------------------------------- + + "AutoClosingIterator" should + "propagate exceptions thrown from onClose (no swallowing)" in { + val it = new AutoClosingIterator[Int]( + Iterator.empty, + () => throw new IllegalStateException("close failed") + ) + val ex = intercept[IllegalStateException] { + it.hasNext + } + assert(ex.getMessage == "close failed") + } + + it should + "still mark itself as closed even when onClose throws (close runs before alreadyClosed is set)" in { Review Comment: The test description says the iterator "still mark[s] itself as closed" when `onClose` throws, but the assertions (and production code order) actually pin the opposite: `alreadyClosed` is not set if `onClose()` throws, so a second `hasNext` re-invokes `onClose`. Renaming the test text will make the intent accurate and easier to maintain. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/scan/AutoClosingIteratorSpec.scala: ########## @@ -0,0 +1,148 @@ +/* + * 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. + */ + +package org.apache.texera.amber.operator.source.scan + +import org.scalatest.flatspec.AnyFlatSpec + +class AutoClosingIteratorSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // hasNext + onClose firing semantics + // --------------------------------------------------------------------------- + + "AutoClosingIterator.hasNext (non-empty underlying)" should + "return true and NOT invoke onClose" in { + var closed = false + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3), () => closed = true) + assert(it.hasNext) + assert(!closed, "onClose must not fire while elements remain") + } + + "AutoClosingIterator.hasNext (exhausted underlying)" should + "return false and invoke onClose exactly once" in { + var closeCount = 0 + val it = new AutoClosingIterator[Int](Iterator.empty, () => closeCount += 1) + assert(!it.hasNext) + assert(closeCount == 1) + } + + it should "NOT invoke onClose again on a second hasNext after exhaustion" in { + var closeCount = 0 + val it = new AutoClosingIterator[Int](Iterator.empty, () => closeCount += 1) + assert(!it.hasNext) + assert(!it.hasNext) + assert(!it.hasNext) + assert(closeCount == 1, s"onClose must fire exactly once, got $closeCount calls") + } + + // --------------------------------------------------------------------------- + // next() — delegates straight through + // --------------------------------------------------------------------------- + + "AutoClosingIterator.next" should "delegate to the wrapped iterator (in order)" in { + val it = new AutoClosingIterator[Int](Iterator(10, 20, 30), () => ()) + assert(it.next() == 10) + assert(it.next() == 20) + assert(it.next() == 30) + } + + // --------------------------------------------------------------------------- + // Full traversal — onClose fires exactly once at the end + // --------------------------------------------------------------------------- + + "AutoClosingIterator full traversal" should + "yield every element of the wrapped iterator in order" in { + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3, 4, 5), () => ()) + assert(it.toList == List(1, 2, 3, 4, 5)) + } + + it should "fire onClose exactly once when toList finishes consuming" in { + var closeCount = 0 + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3), () => closeCount += 1) + val _ = it.toList + assert(closeCount == 1, s"expected single onClose firing, got $closeCount") + } + + // --------------------------------------------------------------------------- + // Already-empty source — close fires on the very first hasNext call + // --------------------------------------------------------------------------- + + "AutoClosingIterator over an already-empty source" should + "fire onClose on the very first hasNext call" in { + var fired = false + val it = new AutoClosingIterator[Int](Iterator.empty, () => fired = true) + val _ = it.hasNext + assert(fired, "onClose must fire when the source is already empty") + } + + // --------------------------------------------------------------------------- + // Mid-iteration close behavior — onClose does NOT fire before exhaustion + // --------------------------------------------------------------------------- + + "AutoClosingIterator (mid-iteration)" should + "leave onClose un-fired between elements (only fires after hasNext returns false)" in { + var fired = false + val it = new AutoClosingIterator[Int](Iterator(1, 2, 3), () => fired = true) + while (it.hasNext) { + it.next() + // After every next(), there is at least one more element OR the + // next hasNext call will fire onClose. Until that point, fired + // must remain false. + // (When the final next() consumes 3, fired is still false because + // the loop hasn't run hasNext again yet.) + } + assert(fired, "after the while loop's final hasNext, onClose must have fired") + } Review Comment: This test currently only asserts that `onClose` fired by the end of iteration, but it does not verify the stated contract that `onClose` stays un-fired *between* elements. As written, it would still pass if `onClose` were incorrectly invoked early (e.g., during a `hasNext` that returns true). Consider rewriting to assert `fired == false` after each `hasNext` that returns true, and only becomes true when `hasNext` first returns false. -- 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]
