Adds replay-tracking option to import_logs script.

[clearcode]

According to http://dev.piwik.org/trac/ticket/3632

[mattab]

Nice! Code Review:

• The Python test script is nice, but to ensure that the test is run as part of CI we should add it to the existing test suite. For example add a test case in this test class: https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Integration/ImportLogsTest.php#L110

it's not "perfect" because the one test tests several features of the import log script, but it's fine for now.
Just add your logs_to_tests.log as a third small log sample and edit this test.

To make tests pass you have to copy from expected/processed folders. See: https://github.com/piwik/piwik/blob/master/tests/README.md#integration-tests

• .decode() can throw UnicodeDecodeError as is already the case in the code at ~ L1318. refactor the decode() or

[clearcode]

Hi, @mattab.

Thanks for your review. We're a bit confused, however, about what you expect as to do in order to make the tests run within CI suite. We've browsed through the code you pointed us at. If we understand correctly you want us to add an additional test method to Test_Piwik_Integration_ImportLogs similar to the one you linked, right? The other thing that confuses us is the "expected/processed folders" thing. We don't see any files mentioning the test you pointed us at. The code, if we understand it correctly, only checks the importer process' exit status.

We had a look at Travis log but we're not sure the tests failed because of our changes. Curiously enough, this and this logs look like everything went fine but they failed anyways. This one looks like some tests failed but we aren't sure if that's due to our changes.

Would you be so kind and provide us with some more details? Bear in mind, however, that our PHP knowledge is somewhat limited :).

[mattab]

FYI this test file also generates 77 xml files test_ImportLogs__*.xml which are tested against the "expected" xml and will find any regression in tracking or API output.
I'll add the test so don't worry about it, it's easy enough.

Also we're still working on Travis sorry for the noise. Soon the builds will be green.

so please make change for catching the exception and it will look good to me.

[clearcode]

Hello,
we've just pushed the code to handle UnicodeDecodeErrors gracefully.

Thanks for the info and your help.

[cbay]

Any reason why the check_ methods have been turned into static methods?

[clearcode]

@cbay this is done to enable us to easily test the script.

[halfdan]

@clearcode, that argument I don't really understand. I am not an expert in Python but: Why do you need static methods to test the script? You can just create an instance and test the method this way.

[clearcode]

@halfdan in order to work on the instance when testing we'd have to create the entire Piwik stack and test the feature using Piwik integration tests. We'd like to avoid doing that in our development enviroment since we've had trouble setting it up. The move to static and class methods allows us to quickly test on example data.

[cbay]

It would be easier to have different merge requests for different features. The replay tracking code is one thing, making the code easier to test is another thing. It makes it harder to review the code if things get mixed up.

Now, regarding the static methods: I don't see why you would need to create the entire Piwik stack with non-static methods. You can just do Parser().check_hostname(hit), rather than check_hostname(hit) (if it were a static), which is not really that much more complicated. Or am I missing something?

[clearcode]

OK, we've managed to revert static methods into instance methods back. The pull request has been updated.

[halfdan]

Thanks guys! Nice commit!