Skip to content

Regenerates Omnifixture #15520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Mar 24, 2020
Merged

Regenerates Omnifixture #15520

merged 11 commits into from
Mar 24, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Feb 6, 2020

It's been about 9 months since the Omnifixture has been regenerated. As there have been a lot changes to various fixtures, there are a lot changes in the exptected UI files. Especially in the tracked visitors & numbers

@sgiehl sgiehl added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Feb 6, 2020
@sgiehl sgiehl added this to the 4.0.0 milestone Feb 6, 2020
@sgiehl
Copy link
Member Author

sgiehl commented Feb 6, 2020

Most tests are working again, but there are a few changes left that don't seem correct.

@tsteur @diosmosis Do you know if that change is correct:
https://builds-artifacts.matomo.org/matomo-org/matomo/omnifixture/38167/UIIntegrationTest_ecommerce_log_segmented.png

The additional action says something about that it shouldn't be tracked in that visit. Not sure if that changed in the last months or if something is broken 🤔

@tsteur
Copy link
Member

tsteur commented Feb 6, 2020

Looking at the FewVisitsWithSetVisitorId it's hard to tell. There is some different visitorId, IP, and User Agent set so would have expected this would be a new visit as also the configId should not match.

@sgiehl
Copy link
Member Author

sgiehl commented Feb 19, 2020 •
edited
Loading

Had another look. Seems that change is related to #14360. Will check and improve the test, so it does what it is intended to.

@sgiehl sgiehl mentioned this pull request Feb 19, 2020
Merged
@sgiehl sgiehl force-pushed the omnifixture branch 6 times, most recently from 9000ebf to 1273b7d Compare February 24, 2020 12:58
@sgiehl sgiehl mentioned this pull request Mar 16, 2020
Merged
@sgiehl sgiehl force-pushed the omnifixture branch 14 times, most recently from 918e4ac to 82db72e Compare March 18, 2020 13:11
@sgiehl sgiehl force-pushed the omnifixture branch 11 times, most recently from 58ad20c to 4867c46 Compare March 23, 2020 12:40
sgiehl
sgiehl commented Mar 23, 2020
View reviewed changes
plugins/CoreHome/javascripts/broadcast.js
Comment on lines -807 to +808
// &segment= value is not sanitized, since segments are designed to accept any user input
if(param != 'segment') {
// &segment= (and &popover=) value is not sanitized, since segments are designed to accept any user input
if(param != 'segment' && param != 'popover') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was required in order to fix a UI test with XSS values. The popover param made problems for row evolution with labels containing special chars. Closing the popover didn't remove the param from url without that change.

@sgiehl
Copy link
Member Author

sgiehl commented Mar 23, 2020

Omnifixture is up to date with latest 4.x-dev. Some tests are still failing.

  • OneClickUpdate -> automatic login after update does not work for any reason (also on 4.x-dev)
  • UIIntegrationTest -> metric tooltip fails randomly - didn't find a good way to fix that properly 🤷‍♂

and two tests using the segment editor are failing because of #15719. Those should work again afterwards

@sgiehl sgiehl marked this pull request as ready for review March 23, 2020 13:20
@sgiehl sgiehl requested a review from diosmosis March 23, 2020 13:20
diosmosis
diosmosis approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files and image changes look good, I guess we can create an issue to fix the OneClickUpdate test? It's probably some issue that needs to be fixed when updating from 3.x to 4.x via one click update.

@sgiehl sgiehl merged commit dc45e00 into 4.x-dev Mar 24, 2020
@sgiehl sgiehl deleted the omnifixture branch March 24, 2020 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis approved these changes

Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sgiehl @tsteur @diosmosis