On 22.09.21 22:18, John Snow wrote:
>
>
> On Fri, Sep 17, 2021 at 7:00 AM Hanna Reitz <hreitz@redhat.com
> <mailto:hreitz@redhat.com>> wrote:
[...]
>
> As you say, run_linters() to me seems very iotests-specific still: It
> emits a specific output that is compared against a reference output.
> Fine for 297, but not fine for a function provided by a
> linters.py, I’d say.
>
> I’d prefer run_linters() to return something like a Map[str,
> Optional[str]], that would map a tool to its output in case of error,
> i.e. ideally:
>
> `{'pylint': None, 'mypy': None}`
>
>
> Splitting the test apart into two sub-tests is completely reasonable.
> Python CI right now has individual tests for pylint, mypy, etc.
>
> 297 could format it something like
>
> ```
> for tool, output in run_linters().items():
> print(f'=== {tool} ===')
> if output is not None:
> print(output)
> ```
>
> Or something.
>
> To check for error, you could put a Python script in python/tests
> that
> checks `any(output is not None for output in
> run_linters().values())` or
> something (and on error print the output).
>
>
> Pulling out run_linters() into an external file and having it print
> something to stdout just seems too iotests-centric to me. I
> suppose as
> long as the return code is right (which this patch is for) it should
> work for Avocado’s simple tests, too (which I don’t like very much
> either, by the way, because they too seem archaic to me), but,
> well. It
> almost seems like the Avocado test should just run ./check then.
>
>
> Yeh. Ideally, we'd just have a mypy.ini and a pylintrc that configures
> the tests adequately, and then 297 (or whomever else) would just call
> the linters which would in turn read the same configuration. This
> series is somewhat of a stop-gap to measure the temperature of the
> room to see how important it was to leave 297 inside of iotests. So, I
> did the conservative thing that's faster to review even if it now
> looks *slightly* fishy.
>
> As for things being archaic or not ... possibly, but why involve extra
> complexity if it isn't warranted?
My opinion is that I find an interface of “prints something to stdout
and returns an integer status code” to be non-intuitive and thus rather
complex actually. That’s why I’d prefer different complexity, namely
one which has a more intuitive interface.
I'm not sure I follow, though, because ultimately what we're trying to do is run terminal commands as part of a broader test suite. Returning status codes and printing output is what they do. We can't escape that paradigm, so is it really necessary to abstract away from it?
I’m also not sure where the extra complexity would be for a
`run_linters() -> Map[str, Optional[str]]`, because 297 just needs the
loop suggested above over `run_linters().items()`, and as for the
Avocado test...
> a simple pass/fail works perfectly well.
I don’t find `any(error_msg for error_msg in run_linters().values())`
much more complex than pass/fail.
(Note: Above, I called it `output`. Probably should have called it
`error_msg` like I did now to clarify that it’s `None` in case of
success and a string otherwise.)
> (And the human can read the output to understand WHY it failed.) If
> you want more rigorous analytics for some reason, we can discuss the
> use cases and figure out how to represent that information, but that's
> definitely a bit beyond scope here.
[...]
> Like, can’t we have a Python script in python/tests that imports
> linters.py, invokes run_linters() and sensibly checks the output? Or,
> you know, at the very least not have run_linters() print anything to
> stdout and not have it return an integer code. linters.py:main()
> can do
> that conversion.
>
>
> Well, I certainly don't want to bother parsing output from python
> tools and trying to make sure that it works sensibly cross-version and
> cross-distro. The return code being 0/non-zero is vastly simpler. Let
> the human read the output log on failure cases to get a sense of what
> exactly went wrong. Is there some reason why parsing the output would
> be beneficial to anyone?
Perhaps there was a misunderstanding here, because there’s no need to
parse the output in my suggestion. `run_linters() -> Map[str,
Optional[str]]` would map a tool name to its potential error output; if
the tool exited successfully (exit code 0), then that `Optional[str]`
error output would be `None`. It would only be something if there was
an error.
Misunderstood based on "checks the output." I might still be approaching this from the standpoint of "I don't see a reason to capture the output" -- beyond letting iotests use it for the diff phase at the end, but I don't think I need to encapsulate it in a return value anywhere for that to happen -- I can just let it print to sys.[stdout|stderr] and let the diff handle the rest, right?
Is there specific value in replicating that 'diff' feature ourselves? We already don't do that, so is it really necessary for me to begin doing it?
> (The Python GitLab CI hooks don't even bother printing output to the
> console unless it returns non-zero, and then it will just print
> whatever it saw. Seems to work well in practice.)
>
>
> Or, something completely different, perhaps my problem is that you
> put
> linters.py as a fully standalone test into the iotests directory,
> without it being an iotest. So, I think I could also agree on
> putting
> linters.py into python/tests, and then having 297 execute that.
> Or you
> know, we just drop 297 altogether, as you suggest in patch 13 – if
> that’s what it takes, then so be it.
>
>
> I can definitely drop 297 if you'd like, and work on making the linter
> configuration more declarative. I erred on the side of less movement
> instead of more so that disruption would be minimal. It might take me
> some extra time to work out how to un-scriptify the test, though. I'd
> like to get a quick temperature check from kwolf on this before I put
> the work in.
So since we seem to want to keep 297, would it be possible to have 297
run a linters.py that’s in python/tests?
Maybe ... I felt like maybe that'd be a bad idea, though, because it puts an iotest-related thing quite far away from the iotests directory. I didn't want anyone to have to hunt for this stuff. I try to explain my case for this a bit better in the commit messages for v2.
I'm sympathetic to the dislike of having something "test-like, but isn't an iotest" in the folder, though, and tried to address that in v2, but I'm not confident it'll be to your satisfaction.
> Hanna
>
>
> PS: Also, summing up processes’ return codes makes me feel not good.
>
>
> That's the part Vladimir didn't like. There was no real reason for it,
> other than it was "easy".
I very much don’t find it easy, because it’s semantically wrong and thus
comparatively hard to understand.
> I can make it a binary 0/1 return instead, if that'd grease the wheels.
Well, while I consider it necessary, it doesn’t really make the patch
more palatable to me.
OK, I am going to send a V2 that may-or-may-not precisely address your core critique, but I think it's quite a bit tidier and goes quite a bit further than what I did here in V1. I think I am still misunderstanding a core complaint here, but I tried to address the things I thought I grokked: Separate mypy and pylint tests, no funky return code manipulation, no iotest prints inside of linters.py, etc. If it's still untenable for you, I'll just have to go from there.
--js