[claudevdm]

#40613 showed that this IO tests were not executing due to missing pytest mark.it_postcommit. Also Fixed failing tests when they actually execute.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.93%. Comparing base (a8d6afd) to head (b286301).
⚠️ Report is 67 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #40618      +/-   ##
==========================================
- Coverage   80.04%   79.93%   -0.12%     
==========================================
  Files         490      493       +3     
  Lines       82916    83441     +525     
==========================================
+ Hits        66373    66695     +322     
- Misses      16543    16746     +203     

Flag	Coverage Δ
python	79.93% <100.00%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claudevdm]

R: @jrmccluskey

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

[jrmccluskey]

A few questions on the review; just out of curiosity, how did the zoneinfo change surface the lack of test execution/breakage here?

[jrmccluskey]

Is there a reason for the refactor of defining the ParDos prior to the pipeline definition both here and elsewhere?

[claudevdm]

No reason, moved back inline.

[jrmccluskey]

Were the typehints applied here incorrect?

[claudevdm]

I think there's an issue with typehinting where if process is annotated to yield TimestampedValue[T] related to #20344

E.g.

from typing import Iterable

import apache_beam as beam
from apache_beam.transforms.window import TimestampedValue


class AddTimestamp(beam.DoFn):
  def process(self, x: int) -> Iterable[TimestampedValue[int]]:
    yield TimestampedValue(x, x)


with beam.Pipeline() as p:
  ints = p | beam.Create([1, 2, 3]) | beam.ParDo(AddTimestamp())
  print('element_type:', ints.element_type)
  _ = ints | beam.Map(lambda x: x + 1).with_input_types(int)

results in

element_type: apache_beam.transforms.window.TimestampedValue[int]
TypeCheckError: The transform 'Map(<lambda ...>)' requires PCollections of type '<class 'int'>'
but was applied to a PCollection of type 'apache_beam.transforms.window.TimestampedValue[int]'

[claudevdm]

A few questions on the review; just out of curiosity, how did the zoneinfo change surface the lack of test execution/breakage here?

https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/08daa781654625f26eeacb10c5bb7adadddffc21/python/src/main/python/bigquery-anomaly-detection/src/bqmonitor/cdc.py is a fork of the bigquery_change_history. So the pyarrow change that broke cdc.py should have also turned these tests red if they were actually running in post commits.

[jrmccluskey]

LGTM, just have to resolve merge issues in CHANGES.md