Add tests for report logging

Lysoun commented 3 years ago

Add tests to ensure the report prints correctly. (Report.print()):

print the correct number of scripts remaining after each step (scan, filter, prune, execute)
when verbose is true, print the names of the scripts remaining after each step (scan, filter, prune, execute)
print the correct number of rules that was checked
when verbose is true, print the names of the rules that were checked
never print logs for a step not reached because Datamaintain is supposed to have failed its execution before reaching the step

As the project grows, options are going to be added to this method and ensuring that it works correctly is very important because logs are the only way that the user is informed about what is happening. While working on a feature for the version 1.3, I added some tests to ensure the feature was correct (https://github.com/4sh/datamaintain/blob/1.3/modules/core/src/test/kotlin/datamaintain/core/report/ReportTest.kt) and I found out that logs were not tested at that point. That is why I created this issue; to ensure that the missing tests will be added at some point.

frankschmitt commented 2 years ago

I'd like to give this one a try. Can you assign it to me, please?

Lysoun commented 2 years ago

Of course!

frankschmitt commented 2 years ago

I must confess it's not 100% clear to me what's missing here. In ExecutorTest.kt, we've got two tests that verify the created report - should execute and build invalid report and should execute and build valid report. So it seems that the report creation is already tested.

Or should the tests ensure that Report.print() works correctly (i.e. given a report with contents XY, it is correctly logged to the logger) ?

Lysoun commented 2 years ago

It's alright if the issue is not clear, feel free to ask questions!

Or should the tests ensure that Report.print() works correctly (i.e. given a report with contents XY, it is correctly logged to the logger) ?

Correct! As the project grows, options are going to be added to this method and ensuring that it works correctly is very important because logs are the only way that the user is informed about what is happening. While working on a feature for the version 1.3, I added some tests to ensure the feature was correct (https://github.com/4sh/datamaintain/blob/1.3/modules/core/src/test/kotlin/datamaintain/core/report/ReportTest.kt) and I found out that logs were not tested at that point. That is why I created this issue; to ensure that the missing tests will be added at some point.

The branch 1.3 will be merged in dev soon, I just have to take care of some conflicts. dev is going to be the version 2.0

I'm going to update the issue description to add all those details. I hope it answers your question @frankschmitt. Thank you so much for being interested in the project, I'm very happy to see an external contributor here 😄

frankschmitt commented 2 years ago

Thanks for the clarification. I derived my branch from dev - should I use 1.3 instead or integrate the changes from 1.3? (I guess cherry-picking ReportTest.kt would also work). Or do you think it makes more sense to integrate your ReportTest.kt and mine when 1.3 (or my feature branch) is merged into dev?

Lysoun commented 2 years ago

There have been significant changes between dev and 1.3 so the best is to use dev. You sure can take some code from the ReportTest in 1.3. Cherry-picking could be complicated because the tests in 1.3 depend on new features in 1.3. I think the safer is to copy/paste the utility code (like how to mock the logger) in your class in your branch and we'll take care of the merge later.

Lysoun commented 2 years ago

Hi @frankschmitt. I have merged the branch 1.3 into dev! If you rebase your branch on dev, you should have everything needed to improve ReportTest 😄

frankschmitt commented 2 years ago

I'd like to make some changes to the existing tests:

rename should not display relative paths when porcelain name is null to should not display relative paths when porcelain is false
provide non-null porcelain names in should not display relative paths when porcelain is false

because the code doesn't really care whether porcelain name is null or not - it only cares about whether the porcelain flag is set. That makes the intention of the test clearer, and the test is symmetrical to the should display relative paths when porcelain is true one.

@Lysoun Is that ok with you?

Lysoun commented 2 years ago

Yes! You're right. Some changes were made in the code and it looks like they were not reflected in the tests.

4sh / datamaintain

Add tests for report logging #155