FrankYang0529 commented on code in PR #73648:
URL: https://github.com/apache/airflow/pull/73648#discussion_r4110676553
##########
go-sdk/pkg/binding/binding_test.go:
##########
@@ -925,14 +925,17 @@ func (s *BindingSuite)
TestResolveStructUnclaimedFromDefaultStaysSilent() {
s.NotContains(logs, "the task handler does not declare")
}
-func (s *BindingSuite) TestResolveStructEmptySpecFailsLoudly() {
+func (s *BindingSuite) TestResolveStructEmptySpecWarns() {
+ // An argless call sends no spec at all, so this is the ordinary shape
of a
+ // stub called as `my_task()`, not a sign of an Airflow that cannot
send one.
fn := func(actx contexttest.Context, input simpleInput) error { return
nil }
for name, args := range map[string][]Arg{"nil-spec": nil, "empty-spec":
{}} {
s.Run(name, func() {
- _, err := s.resolve(fn, args, &fakeXComClient{})
- if s.Assert().Error(err) {
- s.Contains(err.Error(), "no TaskFlow arg
bindings arrived")
- }
+ got, logs, err := s.resolveWithLogs(fn, args,
&fakeXComClient{})
+ s.Require().NoError(err)
Review Comment:
The "Unchanged" paragraph says the task still fails when the spec arrives
empty and the struct has bindable fields, but `TestResolveStructEmptySpecWarns`
only logs warning. How about updating the PR description?
<img width="774" height="75" alt="Image"
src="https://github.com/user-attachments/assets/1495f7cf-2b77-40c0-bebd-9df743c8fcec";
/>
##########
airflow-e2e-tests/tests/airflow_e2e_tests/go_sdk_tests/test_go_sdk_taskflow_binding.py:
##########
@@ -146,21 +162,30 @@ def
test_via_struct_default_arg_tolerates_unclaimed_default(completed_run: _Comp
assert completed_run.xcom("via_struct_default_arg") == {"region":
"eu-west-1"}
-def test_via_struct_more_args_runs_anyway(completed_run: _CompletedRun):
+def test_via_struct_more_args_warns_and_runs(completed_run: _CompletedRun):
"""The call passes ``unused_label``, which the Go struct does not declare.
Name
binding cannot shift, so the extra argument is warned about rather than
failing
the task, and everything the struct does declare still binds."""
assert completed_run.xcom("via_struct_more_args") == {"region":
"eu-west-1"}
+ logs = completed_run.logs("via_struct_more_args")
+ assert "Dag's call passed argument(s) the task handler does not declare"
in logs, logs
+ assert "unused_label" in logs, logs
-def test_via_struct_fewer_args_runs_anyway(completed_run: _CompletedRun):
+def test_via_struct_fewer_args_warns_and_runs(completed_run: _CompletedRun):
"""The Go struct declares ``not_in_dag``, which the stub has no parameter
for. The
field keeps its Go zero value and the mismatch is warned about rather than
failing
- the task, so the two sides can drift without breaking the Dag."""
+ the task, so the two sides can drift without breaking the Dag.
+
+ The warning is the assertion that matters: the zero-valued field alone
would look
+ the same as the older behaviour that filled it silently."""
assert completed_run.xcom("via_struct_fewer_args") == {
"region": "eu-west-1",
"not_in_dag_was_empty": True,
}
+ logs = completed_run.logs("via_struct_fewer_args")
+ assert "Task handler declares argument(s) the Dag's call did not pass" in
logs, logs
+ assert "not_in_dag" in logs, logs
Review Comment:
`ViaStructFewerArgs` logs its own record with a `not_in_dag_was_empty` key ,
and `logs()` joins every record into one string. So the joined string contains
`"not_in_dag"` even if the warning stops naming the struct field. How about
reading the warning record and check its `declared_not_passed` field instead?
```python
records = completed_run.log_records("via_struct_fewer_args")
warning = next(
(r for r in records if r.get("event") == "Task handler declares
argument(s) the Dag's call did not pass"),
None,
)
assert warning is not None, [r.get("event") for r in records]
assert warning.get("declared_not_passed") == ['NotInDag (argument
"not_in_dag")'], warning
```
--
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]
