feat: Debug output for scheduled jobs (--debug in crontab)

[stcksmsh]

Added checkbox to allow debugging to settings>general>schedule in qt and the necessary code to common to make it work.

Fixes #1616

[buhtz]

Thanks a lot for this. Looks good.
In the team we have the rule to keep a PR of our own minimum 1 week open until we merge it without a review of someone else. But for your PR I would like to wait for aryoda's review because he had the original idea. So don't be worried if this PR is open for some weeks. He will reach out someday and he do not lose or ignore notification emails.

Please do not forget the changelog entry in the CHANGES file. Have a look at https://common-changelog.org/ if you want to learn more about writing changelogs. We don't have hard rules or guidelines in the BIT team about writing them. You are free to guess.

Your code looks good. I added only minor suggestions in the other comments.

When it comes to testing. There are two different "schools": London and Detroit (aka classic). None of them are right, good or wrong. It always depends. But for beginners they should try to follow the general rules offered by Detroit school. In that school a "unit" is not code or a code block it is "behavior".

Your PR do introduce a new behavior: The crontab line (generated by the Schedule settings) contain a --debug if the corresponding setting in the GUI has been done.

OK, let's cut the GUI here because it makes it to complex. You would create a unit test like this Pseudocode:

def MyTestSuite(unittest.TestCase):
    def test_crontab_contain_debug():
    
        # create a config instance with a minimal set of options
        conf = config.Config(simple_test_config)
        
        # Test the start conditions: No debug
        self.assertFalse(conf.debugSchedule())
        self.assertFalse(logger.DEBUG)
        
        # The "system under test" (sut)
        sut = conf.cronCmd(profile_id='1')
        self.assertIsNotIn('--debug', sut)
        
        # Invert the situation
        conf.setDebugSchedule(True)
        
        # Test start conditions like this might be pedantic. But my experience
        # tells me it is worth it especially in an old and complex code base
        # like BIT.
        self.assertTrue(conf.debugSchedule())
        
        sut = conf.cronCmd(profile_id='1')       
        self.assertIsIn('--debug', sut)

    def test_crontab_without_debug():
        conf = config.Config(simple_test_config)
        
        conf.setDebugSchedule(True)

        self.assertTrue(conf.debugSchedule())
        self.assertFalse(logger.DEBUG)

        sut = conf.cronCmd(profile_id='1')       
        self.assertIsNotIn('--debug', sut)

One problem with BIT is that the several code parts are not isolated enough from each other. This makes it often hard to nearly impossible to create a valuable unit tests without to much mocking/fakeing/tricking.

Have you ever heard of "test driven development" (TDD)? Your PR is a good example. The behavior I described is what you want. Before starting anything else just write the tests. While writing the test you becoming more clear about the details you need and you don't need. The test will fail in the beginning. When your tests is finished and you are clear about what your needs then you start to implement the code you need to make the test pass (green!).

[aryoda]

@stcksmsh THX a lot for your contribution! I will review, build and test your code in detail next week I hope (I am running out of time ATM).

Meanwhile first observations:

• Would you mind adding the documentation for the new config property into our man page too
• a CHANGELOG is also required (as @buhtz already mentioned above)

[buhtz]

• Would you mind adding the documentation for the new config property into our man page too

That is not needed. The man page for the config file backintime-config.1 is generated automatically based on the comments (starting with #?) used in the getter/setter methods in the config.py file. There is a fancy script (by Germar I think) extracting the comments from the config.py and generate the man page for it.

EDIT: Ah I see you pointed to backintime.1 instead of backintime-config.1. Maybe in the section about the --debug argument could be a hint about this new setting, yes.

[aryoda]

Maybe in the section about the --debug argument could be a hint about this new setting, yes.

This was a "late night" mistake of mine, I meant the backintime-config.1 😄

[buhtz]

Congrats! 🥳 🎈 New tests and they are green. Nice work. I added one comment to the code.

Beside of that I would suggest to move these two tests somewhere else because it IMHO to not really belong into that test suite.

I am not sure how to handle the Config instance in this case. Currently your code use a real config file inside the test folder. And that instance is also used by all other tests. Never a good idea despite that nearly every other BIT test does it the same way. I would try to create an in-memory config file for this test.

I am not sure about a solution. Because Config is not able to read from a "file like" object (e.g. an in-memore io.StringIO instance) I would try to use a fake filesystem using PyFakeFS. Would it be OK for you if I would give a try and taking over from here?

[stcksmsh]

Thanks for the feedback and for offering to handle the changes. Feel free to proceed with moving the tests and implementing the in-memory config file. Let me know if you need any assistance. Looking forward to the improvements!

[buhtz]

My apologize again for blowing up this PR. Discovered a problem with the "tools" module while moving the new tests and then discovered something more.... 🤣 Shouldn't have driven it that far. Kosta: I am a bad example in this case. A PR shouldn't be that big and complex in the end.

Beside the whole PR I do requesting review by @bit-team for two specific aspects of this PR and won't merge it until a I get feedback about it:

1. In short: The debug mode of the GUI was inherited by the scheduled backup jobs. Previously if BIT GUI was started in debug mode (--debug) and the schedule settings are modified the result always was that the resulting crontab line contained a --debug argument (see config.py line 1759/1783). I removed that behavior because it feels not appropriated and find it also quit dangerous because the syslog is filled up without the knowledge of the users. With this PR we now have an explicit setting to achieve the desired behavior.
2. I out-commented 3 lines of code in the method config.py::Config.cronCmd() starting with line 1765. I see no value in it and can not imagine an appropriated use case. But just for sure would like to have your opinion about it.

Beside of this:

• Moved the new tests to their own test file and simplified their code.
• fix: some linter warnings/errors from "codespell" and "pylint".
• fix: PyLint test now fails on any error including pylint-usage errors.
• Based on experience in other projects and the PyLint statistic on BIT's code base (using --reports=yes) I used a code comment to introduce some new pylint rules. They are good candidates to be fixed in the near future without introducing to much code changes.

[buhtz]

Approve on my site. But waiting for other team mates before merging.

[aryoda]

FYI: I plan to build and review the PR end of this week (hopefully ;-)

[buhtz]

• I wrapped the tool tip.
• Separated it into two paragraphs to realize a line break before "Caution".
• Replaced ! with . because once a professional GUI translator told me one should not shout at or pressure users. That's roughly how I understood it. 😄

[aryoda]

OK, this PR is now ready to be merged into dev (I'd suggest to wait a few "cooling-off" days before merging).

Thanks a lot to @stcksmsh for this great contribution and to @buhtz for the refinements - excellent team work 👍