dstandish commented on PR #28710:
URL: https://github.com/apache/airflow/pull/28710#issuecomment-1373423090
ok i think i have gotten the test issue fixed now... it was the result of
reloading the s3 module. after that, mocks don't work.
separately i had a chance to look at the change @feluelle and i reverted, in
small part cus it was failing :) but mainly because...
this is sort of why i objected to doing it that way.
with your way, we see a bunch of test values but we don't really know what
they are there for. sure you could use param class and add an ID but that is a
lot more noise. and it risks that the params differ from the description (id).
meanwhile, my params, to borrow a phrase, "descriptive and meaningful
phrases", e.g.
`"unify", "no_conn", "no_bucket", "full_key"`
this scenario covers the case unify first, with no connection (or no schema
in connection), and no bucket provided as kwarg, and with a full key.
meanwhile, with yours we see
```None, {"key": "s3://key_bucket/key.txt"}```
if we look at this, how are we to know what it's testing? and when we look
at all the params together, how do we know if we have missed a case?
with mine, we just need to verify that we all combinations of the 4 binary
choices. when using the values directly, they don't quite combine that way.
and, this is also related to why i didn't want to make them booleans, which
@o-nikolas suggested. by using the string values, we get good test names. test
names that clearly indicate the scenario tested.
anyway, not that these other approaches aren't fine, just explaining my
choices here.
thanks
--
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]
