@sgiehl opened this Pull Request on June 22nd 2021 Member

Description:

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@sgiehl commented on June 25th 2021 Member

Note: One of the new tests is currently failing due to https://github.com/matomo-org/matomo-log-analytics/pull/317

@diosmosis commented on June 27th 2021 Member

Nice test addition :+1:! Would it also be good to have a replay log for the test? There we can also include strange encoding for the action_name.

Also think it might be a good idea to regenerate OmniFixture at some point after this so we can see how the UI tests respond. What do you think?

@sgiehl commented on June 28th 2021 Member

@diosmosis
replay log should be covered by this: https://github.com/matomo-org/matomo/blob/375dd9f6933dcf7eeb37df9d2d9f44d16f35a277/tests/PHPUnit/Fixtures/ManySitesImportedLogs.php#L278-L287

And the added tests won't change the OmniFixture, as the visits are imported within single tests and not within a Fixture.

@diosmosis commented on June 28th 2021 Member

@sgiehl

replay log should be covered by this:

I meant adding the encoding to the reply logs so an encoded value can be added to action_name for tests.

And the added tests won't change the OmniFixture, as the visits are imported within single tests and not within a Fixture.

Would it be better if it was within a fixture so OmniFixture would pick it up? It might be useful to see if those types of encoded values breaks anything in the UI.

@sgiehl commented on July 1st 2021 Member

@diosmosis I won't change anything here for now, as there is more important stuff to work on. Maybe you can have a quick final look and merge the additional tests if they are fine. We can consider changing them later as well.

@diosmosis commented on July 1st 2021 Member

:+1: I'll make a new issue for adding encoded data to fixtures

This Pull Request was closed on July 1st 2021
Powered by GitHub Issue Mirror