Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[Hanna Reitz <hreitz@redhat.com>]

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 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.

> (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?

>     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.

Hanna