@sgiehl opened this Pull Request on April 7th 2021 Member

Description:

The PR also includes some changes to JS tests, so they don't fail in a browser that supports sendBeacon

requires https://github.com/matomo-org/travis-scripts/pull/68

fixes #17424

~Before merging this on, the travis-scripts PR needs to be merged and the submodule reference in this PR needs an update.~

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
@tsteur commented on April 7th 2021 Member

@sgiehl I'm almost thinking maybe we don't want this just yet as it may be more valuable to know that the tracking works in older browsers and then most likely it also works in newer browsers. Vs if we run it in a newer browser it's harder to ensure it works in an older browser. We may not want this change just yet maybe.

@Findus23 commented on April 7th 2021 Member

I think the Javascript test is one of the fastest (taking just a few minutes). So maybe it might be possible to run both.

@diosmosis commented on April 7th 2021 Member

Just saw this after looking at the travis-scripts PR. I can revert the merge I did if needed.

@sgiehl commented on April 8th 2021 Member

running both sounds fair enough. should be simple to do that. will update this PR

@sgiehl commented on April 8th 2021 Member

Updated the PR so the travis job should first run the tests on node and afterwards on phantomjs

This Pull Request was closed on April 11th 2021
Powered by GitHub Issue Mirror