You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/02 14:37:07 UTC
[GitHub] [beam] jrmccluskey opened a new pull request, #17813: [BEAM-14546] Fix errant pass for empty collections in Count
jrmccluskey opened a new pull request, #17813:
URL: https://github.com/apache/beam/pull/17813
Adds a NonEmpty() passert utility to check that a given PCollection has at least one element, then uses it to ensure that a collection passed to Count() with a non-zero expected number of elements has at least one element to avoid an erroneous passing test.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145127975
Run Go Flink ValidatesRunner
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1144961055
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145180754
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145001536
The portable failure looks legit as well:
```
11:01:07 2022/06/02 14:58:41 2022-06-02 14:58:41. (26): Traceback (most recent call last):
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/local_job_service.py", line 275, in _run_job
11:01:07 self.result = self._invoke_runner()
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/local_job_service.py", line 297, in _invoke_runner
11:01:07 return fn_runner.FnApiRunner(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 208, in run_via_runner_api
11:01:07 return self.run_stages(stage_context, stages)
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 408, in run_stages
11:01:07 bundle_results = self._execute_bundle(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 736, in _execute_bundle
11:01:07 self._run_bundle(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 965, in _run_bundle
11:01:07 result, splits = bundle_manager.process_bundle(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 1314, in process_bundle
11:01:07 raise RuntimeError(result.error)
11:01:07 RuntimeError: process bundle failed for instruction bundle_15 using plan 2 : while executing Process for Plan[2]:
11:01:07 2: ParDo[passert.nonEmptyFn] Out:[]
11:01:07 1: DataSource[S[passert.Count(out)/passert.NonEmpty/Impulse@localhost:38365], i0] Coder:W;c0_windowed<bytes;c0>!GWC Out:2
11:01:07 caused by:
11:01:07 DoFn[UID:2, PID:passert.Count(out)/passert.NonEmpty/passert.nonEmptyFn, Name: github.com/apache/beam/sdks/v2/go/pkg/beam/testing/passert.nonEmptyFn] failed:
11:01:07 PCollection is empty, want non-empty collection
11:01:07 2022/06/02 14:58:41 2022-06-02 14:58:41. (27): Error running pipeline.
11:01:07 Traceback (most recent call last):
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/local_job_service.py", line 275, in _run_job
11:01:07 self.result = self._invoke_runner()
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/local_job_service.py", line 297, in _invoke_runner
11:01:07 return fn_runner.FnApiRunner(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 208, in run_via_runner_api
11:01:07 return self.run_stages(stage_context, stages)
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 408, in run_stages
11:01:07 bundle_results = self._execute_bundle(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 736, in _execute_bundle
11:01:07 self._run_bundle(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 965, in _run_bundle
11:01:07 result, splits = bundle_manager.process_bundle(
11:01:07 File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_GoPortable_Commit/src/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py", line 1314, in process_bundle
11:01:07 raise RuntimeError(result.error)
11:01:07 RuntimeError: process bundle failed for instruction bundle_15 using plan 2 : while executing Process for Plan[2]:
11:01:07 2: ParDo[passert.nonEmptyFn] Out:[]
11:01:07 1: DataSource[S[passert.Count(out)/passert.NonEmpty/Impulse@localhost:38365], i0] Coder:W;c0_windowed<bytes;c0>!GWC Out:2
11:01:07 caused by:
11:01:07 DoFn[UID:2, PID:passert.Count(out)/passert.NonEmpty/passert.nonEmptyFn, Name: github.com/apache/beam/sdks/v2/go/pkg/beam/testing/passert.nonEmptyFn] failed:
11:01:07 PCollection is empty, want non-empty collection
11:01:07 2022/06/02 14:58:41 Job state: FAILED
11:01:07 ptest.go:108: Failed to execute job: job go-testsimplepipeline-e0c742f9-4ac7-4f8f-9eff-73c54d6d8617 failed
11:01:07 --- FAIL: TestSimplePipeline (12.70s)
```
Not sure if the test is broken (and erroneously passing) or if something here is broken
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145051473
Run Go Flink ValidatesRunner
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on a diff in pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on code in PR #17813:
URL: https://github.com/apache/beam/pull/17813#discussion_r888189540
##########
sdks/go/pkg/beam/testing/passert/count.go:
##########
@@ -30,6 +30,10 @@ func Count(s beam.Scope, col beam.PCollection, name string, count int) {
if typex.IsKV(col.Type()) {
col = beam.DropKey(s, col)
}
+
+ if count > 0 {
+ NonEmpty(s, col)
Review Comment:
Sum did need it, confirmed via unit test. Will add some short validation for Hash
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1144972749
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @damccorm for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1144959989
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on a diff in pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on code in PR #17813:
URL: https://github.com/apache/beam/pull/17813#discussion_r888216256
##########
sdks/go/pkg/beam/testing/passert/count_test.go:
##########
@@ -22,24 +22,62 @@ import (
"github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
)
-func TestCount_Good(t *testing.T) {
- p, s := beam.NewPipelineWithRoot()
- col := beam.Create(s, "a", "b", "c", "d", "e")
- count := 5
+func TestCount(t *testing.T) {
+ var tests = []struct {
+ name string
+ elements []string
+ count int
+ }{
+ {
+ "full",
+ []string{"a", "b", "c", "d", "e"},
+ 5,
+ },
+ {
+ "empty",
+ []string{},
+ 0,
+ },
+ }
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ p, s := beam.NewPipelineWithRoot()
+ col := beam.CreateList(s, test.elements)
- Count(s, col, "TestCount_Good", count)
- if err := ptest.Run(p); err != nil {
- t.Errorf("Pipeline failed: %v", err)
+ Count(s, col, test.name, test.count)
+ if err := ptest.Run(p); err != nil {
+ t.Errorf("Pipeline failed: %v", err)
+ }
+ })
}
}
func TestCount_Bad(t *testing.T) {
Review Comment:
We could, although I don't see too much value in bundling them into one suite. The setup isn't particularly long or complicated to test the function, so deduplicating it doesn't add much value IMO. Totally cool doing it if you feel strongly though.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145051756
Run Go PostCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145146022
R: @youngoli for final approval
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17813:
URL: https://github.com/apache/beam/pull/17813#discussion_r888079762
##########
sdks/go/pkg/beam/testing/passert/count.go:
##########
@@ -30,6 +30,10 @@ func Count(s beam.Scope, col beam.PCollection, name string, count int) {
if typex.IsKV(col.Type()) {
col = beam.DropKey(s, col)
}
+
+ if count > 0 {
+ NonEmpty(s, col)
Review Comment:
Do we need to add the same thing to [Hash](https://github.com/apache/beam/blob/9e986e74efff2a7293ec5c7a86917479882d9590/sdks/go/pkg/beam/testing/passert/hash.go#L36) and [Sum](https://github.com/apache/beam/blob/9e986e74efff2a7293ec5c7a86917479882d9590/sdks/go/pkg/beam/testing/passert/sum.go#L33)? I think an empty pcollection would silently pass for both of those as well
##########
sdks/go/pkg/beam/testing/passert/count_test.go:
##########
@@ -22,24 +22,62 @@ import (
"github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
)
-func TestCount_Good(t *testing.T) {
- p, s := beam.NewPipelineWithRoot()
- col := beam.Create(s, "a", "b", "c", "d", "e")
- count := 5
+func TestCount(t *testing.T) {
+ var tests = []struct {
+ name string
+ elements []string
+ count int
+ }{
+ {
+ "full",
+ []string{"a", "b", "c", "d", "e"},
+ 5,
+ },
+ {
+ "empty",
+ []string{},
+ 0,
+ },
+ }
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ p, s := beam.NewPipelineWithRoot()
+ col := beam.CreateList(s, test.elements)
- Count(s, col, "TestCount_Good", count)
- if err := ptest.Run(p); err != nil {
- t.Errorf("Pipeline failed: %v", err)
+ Count(s, col, test.name, test.count)
+ if err := ptest.Run(p); err != nil {
+ t.Errorf("Pipeline failed: %v", err)
+ }
+ })
}
}
func TestCount_Bad(t *testing.T) {
Review Comment:
Can we simplify and combine these tests into 1 by adding a `expectErr` variable to the tests struct?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145098548
Found the problem, the synthetic StepCfg struct was not exported so the number of elements to emit in the synthetic tests was getting set to 0, producing empty PCollections
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on a diff in pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on code in PR #17813:
URL: https://github.com/apache/beam/pull/17813#discussion_r888212697
##########
sdks/go/pkg/beam/testing/passert/count.go:
##########
@@ -30,6 +30,10 @@ func Count(s beam.Scope, col beam.PCollection, name string, count int) {
if typex.IsKV(col.Type()) {
col = beam.DropKey(s, col)
}
+
+ if count > 0 {
+ NonEmpty(s, col)
Review Comment:
Added a small "fail on empty" check for Hash.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145987212
Run GoPortable PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1144946363
# [Codecov](https://codecov.io/gh/apache/beam/pull/17813?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17813](https://codecov.io/gh/apache/beam/pull/17813?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d8d897e) into [master](https://codecov.io/gh/apache/beam/commit/999bceab8e87d25f30faffe7d6431e2d8588663f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (999bcea) will **increase** coverage by `0.00%`.
> The diff coverage is `82.00%`.
```diff
@@ Coverage Diff @@
## master #17813 +/- ##
=======================================
Coverage 74.09% 74.09%
=======================================
Files 697 697
Lines 91980 92025 +45
=======================================
+ Hits 68148 68187 +39
- Misses 22583 22589 +6
Partials 1249 1249
```
| Flag | Coverage Δ | |
|---|---|---|
| go | `50.88% <82.00%> (+0.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/17813?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [sdks/go/pkg/beam/testing/passert/passert.shims.go](https://codecov.io/gh/apache/beam/pull/17813/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS90ZXN0aW5nL3Bhc3NlcnQvcGFzc2VydC5zaGltcy5nbw==) | `59.47% <79.31%> (+2.24%)` | :arrow_up: |
| [sdks/go/pkg/beam/testing/passert/count.go](https://codecov.io/gh/apache/beam/pull/17813/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS90ZXN0aW5nL3Bhc3NlcnQvY291bnQuZ28=) | `79.16% <100.00%> (+2.97%)` | :arrow_up: |
| [sdks/go/pkg/beam/testing/passert/passert.go](https://codecov.io/gh/apache/beam/pull/17813/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS90ZXN0aW5nL3Bhc3NlcnQvcGFzc2VydC5nbw==) | `81.11% <100.00%> (+2.36%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17813?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17813?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [999bcea...d8d897e](https://codecov.io/gh/apache/beam/pull/17813?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17813:
URL: https://github.com/apache/beam/pull/17813#discussion_r888219278
##########
sdks/go/pkg/beam/testing/passert/count_test.go:
##########
@@ -22,24 +22,62 @@ import (
"github.com/apache/beam/sdks/v2/go/pkg/beam/testing/ptest"
)
-func TestCount_Good(t *testing.T) {
- p, s := beam.NewPipelineWithRoot()
- col := beam.Create(s, "a", "b", "c", "d", "e")
- count := 5
+func TestCount(t *testing.T) {
+ var tests = []struct {
+ name string
+ elements []string
+ count int
+ }{
+ {
+ "full",
+ []string{"a", "b", "c", "d", "e"},
+ 5,
+ },
+ {
+ "empty",
+ []string{},
+ 0,
+ },
+ }
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ p, s := beam.NewPipelineWithRoot()
+ col := beam.CreateList(s, test.elements)
- Count(s, col, "TestCount_Good", count)
- if err := ptest.Run(p); err != nil {
- t.Errorf("Pipeline failed: %v", err)
+ Count(s, col, test.name, test.count)
+ if err := ptest.Run(p); err != nil {
+ t.Errorf("Pipeline failed: %v", err)
+ }
+ })
}
}
func TestCount_Bad(t *testing.T) {
Review Comment:
I'd argue its worth doing since more/duplicated code => more opportunities for bugs to slip in when updates are needed and more for a future developer (maybe us) to understand ([code is a liability](https://blog.devgenius.io/code-is-a-liability-to-developers-e6ffade2b7d0)). I'm not going to block on it though, its not very important
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145051272
It's strange, from what I can tell the problem lies in the synthetic code since Count() gets used heavily in a number of places and we have both unit testing and the other integration tests passing. Let me see if that's reproducible on other runners
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jrmccluskey commented on pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
jrmccluskey commented on PR #17813:
URL: https://github.com/apache/beam/pull/17813#issuecomment-1145178858
R: @lostluck since they have context
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck merged pull request #17813: [BEAM-14546] Fix errant pass for empty collections in Count
Posted by GitBox <gi...@apache.org>.
lostluck merged PR #17813:
URL: https://github.com/apache/beam/pull/17813
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org