Test `ignore_additional_command_line_args` option

[meliache]

Tests for the ignore_additional_command_line_args option to b2luigi.process introduced in #55 by @klieret. Then I will have more peace of mind when people claim to have issues with this option.

If ignore_additional_command_line_args=True, b2luigi.process should allow additional command line args not known to b2luigi, as they might be used by the user script. Otherwise, only b2luigi cli args should be allowed.

In this PR there is a bit of boilerplate code due to DoNothingTask being defined twice in two test-scripts. Somehow I had issues with using relative import in the test-directory since it's not a package. Originally I had them in a cli_tasks.py file in the same directory and imported them with

from .cli_tasks import DoNothingTask

But this didn't work when running the unit tests. Not sure why that is not working but import ..helpers works, maybe I should add an __init__.py or something like that? Maybe @nils-braun or someone else knows?

[klieret]

Maybe you should force everyone who does a feature PR to also write a test for it :)

[klieret]

Oh and my 2 cents regarding relative imports: I can't think of any advantage, only of trouble I've experienced with them :sweat_smile:

Maybe you can give https://github.com/MarcoGorelli/absolufy-imports a shot?

[meliache]

@klieret

Maybe you should force everyone who does a feature PR to also write a test for it :)

Since recently we have code-coverage checks in our CI and since then I try to enforce that more. At least when it comes to b2luigi core features.

Exceptions where I sometimes accept no unittests are some batch-specific minor features like adding a new setting for a batch process that is then passed e.g. as a CLI argument to it, something like this can be hard to unit-test (but still possible), as it requires to mock the batch system behavior.

[meliache]

@klieret Regarding relative imports, I usually never use them in python package code, but the tests directory isn't a module of the b2luigi package, so I can't import it. absolufy-imports converts my relative import statement (not working) to

from tests.cli.cli_tasks import DoNothingTask

That doesn't make sense, I would expect rather something like from b2luigi.tests.cli.cli_tasks import ..., but both of those result in ModuleNotFoundError's.

Still, in general absolufy-imports a nice package that could be added to our pre-commit definitions, since self-fixing hooks rock :)

[klieret]

Ah, speaking of hooks: I'm surprised you're not using the CI for pre-commit to check them in PRs, as it's as easy as 3 mouse clicks: http://pre-commit.ci/

[meliache]

@klieret We are using pre-commit hooks to check PR's but it's a task in the test.yml github action and not a separate action. However I didn't do this via 3 clicks but by manually adding the task to the yaml, so I will take a look at the pre-commit-ci app and see if we can do that in a "better" way. Maybe also with a separate shield in the readme to make it more obvious that we use pre-commit.

The only downside is that sometimes I get PR's that follow with multiple "fix style" commits because instead of installing pre-commit locally, first-time contributers often push commits until pre-commit checks pass on github, despite the instructions in the development docs.

Edit: I tried the 3-click pre-commit-app UI but it seems to only work for repositories that I own :thinking:. In b2luigi I dont' have access to repository settings and etc, though I can edit the github actions via PRs.