[I] FlowControlSpec's size-cap test asserts nothing and its name claims a guarantee the file does not pin [texera]

["aglinxinyuan (via GitHub)" <gi...@apache.org>]

aglinxinyuan opened a new issue, #8402:
URL: https://github.com/apache/texera/issues/8402

   ### Task Summary
   
   `FlowControlSpec` contains a test whose name claims a guarantee nothing in the file pins:
   
   ```scala
   "FlowControl" should "trip the size-cap assertion for a message that exceeds maxByteAllowed" in {
     // ...
     val fc = new FlowControl()
     (1L to 1000L).foreach(i => fc.getMessagesToSend(msg(i)))
     succeed
   }
   ```
   
   It ends in a bare `succeed` and checks no property of the result. Its own comment concedes it cannot synthesize an oversized payload, then claims to "lock down the guard shape" by sending 1000 messages — but nothing about those 1000 calls is asserted.
   
   The guard it is named after is `assert(creditNeeded <= maxByteAllowed, ...)` at the top of `FlowControl.getMessagesToSend`. Deleting that block from the production file leaves all 14 tests in `FlowControlSpec` green. The fixture cannot reach it: `FixedSizePayload` reports 200 bytes against a configured cap of 1,600,000,000, and 1000 x 200 = 200,000 bytes does not exhaust credit either, so the messages never take the stashing path.
   
   The guard is reachable from a unit test, contrary to what the test's comment asserts. `DataFrame.inMemSize` is `frame.map(_.inMemSize).sum`, which does not deduplicate, so an `Array[Tuple]` holding the same tuple reference N times reports N times its size — a payload over a multi-GB cap costs a few hundred KB of real memory. No production seam is needed.
   
   Proposed cleanup, test-only:
   
   - add a test that actually trips the guard with such a payload, asserting the `AssertionError` and that the rejection leaves the channel untouched (credit unchanged, not marked overloaded — distinguishing the guard from the out-of-credit branch below it);
   - replace the assertion-free body with one that pins the fast path the guard sits on: what `getMessagesToSend` returns per under-cap message and the resulting `getCredit` / `isOverloaded` state across a batch, which is what catches accounting that is right for the first message and then drifts;
   - derive expected values from the fixture (`msgSize` from `WorkflowMessage.getInMemSize`, `maxBytes` from `ApplicationConfig`) rather than hard-coding them, plus a precondition that the batch fits under the cap;
   - rewrite the stale comment.
   
   No production change and no reformatting; one test file. Two unrelated weaknesses in neighbouring tests in the same file — a tautological `assert(seen == stashed.size)` in the multi-run drain test, and a suite-constructor `assert(msgSize == 200L)` that would abort the suite rather than fail a test — should be handled separately, not folded into this cleanup.
   
   ### Task Type
   
   - [x] Refactor / Cleanup
   
   ### Was this issue authored using generative AI tooling?
   
   Generated-by: Claude Code (Opus 5)
   


-- 
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: notifications-unsubscribe@texera.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] FlowControlSpec's size-cap test asserts nothing and its name claims a guarantee the file does not pin [texera]

["aglinxinyuan (via GitHub)" <gi...@apache.org>]

aglinxinyuan closed issue #8402: FlowControlSpec's size-cap test asserts nothing and its name claims a guarantee the file does not pin
URL: https://github.com/apache/texera/issues/8402


-- 
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: notifications-unsubscribe@texera.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org