@clearcode opened this Pull Request on February 13th 2013
@mattab commented on February 14th 2013 Member

Nice! Code Review:

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 commented on February 14th 2013

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 commented on February 14th 2013 Member

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 commented on February 15th 2013

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

Thanks for the info and your help.

@cbay commented on February 15th 2013 Contributor

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

@clearcode commented on February 15th 2013

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

@halfdan commented on February 15th 2013 Member

@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 commented on February 15th 2013

@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 commented on February 15th 2013 Contributor

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 commented on February 20th 2013

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

@halfdan commented on February 20th 2013 Member

Thanks guys! Nice commit!

This Pull Request was closed on February 20th 2013
Powered by GitHub Issue Mirror