You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "Yunyung (via GitHub)" <gi...@apache.org> on 2024/10/23 10:48:36 UTC
[PR] add tests for `ReadOnlyWindowStoreStub#fetch(null, null ...)` [kafka]
Yunyung opened a new pull request, #17586:
URL: https://github.com/apache/kafka/pull/17586
https://github.com/apache/kafka/pull/17540 fixed the **incorrect check**, but our test cases didn't well test this incorrect code
PR [#17540](https://github.com/apache/kafka/pull/17540) orrected the check, but the test cases did not correctly capture the incorrect check before the PR was merged. This is because the expected result asserted in `shouldFetchKeyRangeAcrossStoresWithNullKeyFrom()` coincidentally matched the result obtained at [line 272](https://github.com/apache/kafka/pull/17540/files#diff-77f6b10146a50b25e538ab71c9ffe18fd981d717b666c94d7c811af690cea1eeR272).
To demonstrate what this change covers, I reverted the changes from [#17540](https://github.com/apache/kafka/pull/17540) and applied my fix. The test fails as expected, as shown below:
```
$ ./gradlew streams:test --tests CompositeReadOnlyWindowStoreTest
----snip----
org.apache.kafka.streams.state.internals.CompositeReadOnlyWindowStoreTest.shouldFetchKeyRangeAcrossStoresWithNullKeyFrom() failed, log available in /home/yungh/kafka/kafka_fork/kafka/streams/build/reports/testOutput/org.apache.kafka.streams.state.internals.CompositeReadOnlyWindowStoreTest.shouldFetchKeyRangeAcrossStoresWithNullKeyFrom().test.stdout
Gradle Test Run :streams:test > Gradle Test Executor 57 > CompositeReadOnlyWindowStoreTest > shouldFetchKeyRangeAcrossStoresWithNullKeyFrom() FAILED
java.lang.AssertionError:
Expected: <[KeyValue([a@0/30000], a), KeyValue([b@10/30010], b)]>
but: was <[KeyValue([a@0/30000], a), KeyValue([b@10/30010], b), KeyValue([c@10/30010], c)]>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
at org.apache.kafka.streams.state.internals.CompositeReadOnlyWindowStoreTest.shouldFetchKeyRangeAcrossStoresWithNullKeyFrom(CompositeReadOnlyWindowStoreTest.java:380)
----snip----
org.apache.kafka.streams.state.internals.CompositeReadOnlyWindowStoreTest.shouldBackwardFetchKeyRangeAcrossStoresWithNullKeyFrom() failed, log available in /home/yungh/kafka/kafka_fork/kafka/streams/build/reports/testOutput/org.apache.kafka.streams.state.internals.CompositeReadOnlyWindowStoreTest.shouldBackwardFetchKeyRangeAcrossStoresWithNullKeyFrom().test.stdout
Gradle Test Run :streams:test > Gradle Test Executor 57 > CompositeReadOnlyWindowStoreTest > shouldBackwardFetchKeyRangeAcrossStoresWithNullKeyFrom() FAILED
java.lang.AssertionError:
Expected: <[KeyValue([a@0/30000], a), KeyValue([b@10/30010], b)]>
but: was <[KeyValue([a@0/30000], a), KeyValue([c@10/30010], c), KeyValue([b@10/30010], b)]>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
at org.apache.kafka.streams.state.internals.CompositeReadOnlyWindowStoreTest.shouldBackwardFetchKeyRangeAcrossStoresWithNullKeyFrom(CompositeReadOnlyWindowStoreTest.java:423)
----snip----
32 tests completed, 2 failed
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':streams:test'.
> There were failing tests. See the report at: file:///home/yungh/kafka/kafka_fork/kafka/streams/build/reports/tests/test/index.html
BUILD FAILED in 6s
97 actionable tasks: 3 executed, 94 up-to-date
```
The **incorrect check** can be detected after applying my change.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17854 Improve tests for ReadOnlyWindowStoreStub#fetch and #backwardFetch [kafka]
Posted by "Yunyung (via GitHub)" <gi...@apache.org>.
Yunyung commented on PR #17586:
URL: https://github.com/apache/kafka/pull/17586#issuecomment-2436124669
Thanks for review :)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17854: add tests for `ReadOnlyWindowStoreStub#fetch(null, null ...)` [kafka]
Posted by "Yunyung (via GitHub)" <gi...@apache.org>.
Yunyung commented on PR #17586:
URL: https://github.com/apache/kafka/pull/17586#issuecomment-2431724994
@chia7712
If this change makes sense to you, should we update the Jira and Git commit headers and descriptions?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17854: add tests for `ReadOnlyWindowStoreStub#fetch(null, null ...)` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #17586:
URL: https://github.com/apache/kafka/pull/17586
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17854: add tests for `ReadOnlyWindowStoreStub#fetch(null, null ...)` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #17586:
URL: https://github.com/apache/kafka/pull/17586#issuecomment-2436078887
> If this change makes sense to you, should we update the Jira and Git commit headers and descriptions to something like Improve tests for ReadOnlyWindowStoreStub#fetch and #backwardFetch?
yes, you are right. I will update them after merging this PR
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org