Add option to output stats file live, i.e. after each page crawled

[benoit74]

Fix #358

Changes

• new CLI parameter liveStatsFile
• when CLI parameter is True, stats file is updated after each page crawled

To be discussed

• do we really need this new CLI parameter ? This is has been done so to ensure backward compatibility but we could also revert to previous crawler behavior where if --statsFilename is set, the stats file is updated after each page crawled. As mentioned in the issue, it is not clear for us @kiwix why this behavior has been changed

[benoit74]

@Chickensoupwithrice I hope you did not already started to work on this one, I just realized you assign the issue to yourself, sorry if this is duplicate work

[benoit74]

FYI, this has been manually tested locally with success:

--statsFilename	--liveStatsFile	result
not set	not set	OK, stats file is not written
set	not set	OK, stats file is written only once crawling is done
set	set	OK, stats file is written after each page crawled
not set	set	OK, stats file is not written

[Chickensoupwithrice]

I had not started work on this, so it's not duplicate! Looks good, will test locally and review! :)

[ikreymer]

do we really need this new CLI parameter ? This is has been done so to ensure backward compatibility but we could also revert to previous crawler behavior where if --statsFilename is set, the stats file is updated after each page crawled. As mentioned in the issue, it is not clear for us https://github.com/kiwix why this behavior has been changed

Hi @benoit74 thanks for the PR and tracking this down! You are right, this was an accidental regression, as we don't use the write stats after every page feature ourselves as much and don't have a proper test for it.
I think we can revert to the previous functionality with --statsFilename and just remove the toFile flag there.
If you have a chance to add a test for this, that would be even better, otherwise just reverting to previous functionality should work. Thanks again!

[benoit74]

Thank you for the feedback, I will do it tomorrow (remove the `toFile` flag, revert the additional CLI flag and add a test).

…

Message ID: ***@***.*** com>

[benoit74]

Code is ready, I added a test which checks that the stat file is written and contains proper data.

Unfortunately I wasn't able to write a test that checks that stat file is written after only one page crawled (and not only at the end), I have no idea about how to "pause" the crawl after only some page is crawled (and do not consider that crawl is finished), so that one can check that stat file is there.

While running the tests I also realized that there was a small inversion between expect and test values in extra_hop_depth test suite so I fixed this (test was failing because I did not realized that I must clean the test-crawls folder before starting Jest.

Btw, I don't know if this is intentional or not, and I don't want to be pushy on this, it took us a while to decide on this point for our repos, but your CI workflow is configured to trigger only on "push" events, meaning that it does not trigger on PRs like this one where the push is made on a fork. If you would like to fix this, you could have a look at our convention at https://github.com/openzim/_python-bootstrap/blob/main/.github/workflows/QA.yaml ; workflow is triggered on PRs (because it is usually mandatory in our process) and push on main branch (to "catch them all").

[ikreymer]

Thanks! Yes, we can enable CI on PR as well, think it was also an oversight on this repo.

For testing, what you could do is use exec instead of execSync, and check the contents of the file to see if it changes while a small crawl is running. If the file changes at least once, while crawl is running, we know its being updated..

We can merge this now, if you'd like additional testing, feel free to open another PR with improved test. (This feature is really only used by Kiwix so up to you if you want more extensive testing but we're having to accept it).

[benoit74]

Great, thank you !
Do you have any idea when the next release including this change will happen? (no real hurry, it is more a planning question for us)