Skip to content
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.

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

Updates some npm dependencies for screenshot tests #17663

Merged
merged 1 commit into from Jun 11, 2021
Merged

Updates some npm dependencies for screenshot tests #17663

merged 1 commit into from Jun 11, 2021

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 10, 2021

Description:

Some of the npm dependencies used for screenshot testing keep triggering security warnings.
Even though they aren't used in production it shouldn't hurt to update at least some of them.

Updating mocha might also be useful at some point as it is quite outdated, but that will require various adjustments in our code...

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 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. Needs Review PRs that need a code review labels Jun 10, 2021
@Findus23
Copy link
Member

I guess updating puppeteer should also be useful as Chrome 80.0.3987.0 is quite old now. Maybe we could even automate this somehow so that we notice things that break in newer browsers.

@sgiehl
Copy link
Member Author

sgiehl commented Jun 10, 2021

The problem with updating puppeteer is that in most cases all screenshots are changing slightly, so we actually can't automate it.
And doing the update takes some time, as you need to look through all changing screenshots in order to check if all changes are "valid". Might be useful to create an issue for that, so we can prioritize that.

@diosmosis
Copy link
Member

I guess we could have a github action that does it, say once a month, and just auto commits all the changed screenshots, but only creates a PR. Then someone can review each screenshot and make changes or merge. Would that work?

@diosmosis diosmosis added this to the 4.4.0 milestone Jun 11, 2021
@diosmosis diosmosis merged commit 5e9df52 into 4.x-dev Jun 11, 2021
@diosmosis diosmosis deleted the npmupdateui branch June 11, 2021 00:18
@sgiehl
Copy link
Member Author

sgiehl commented Jun 11, 2021

@diosmosis guess that should work. Might be best to create an issue for it

@diosmosis
Copy link
Member

Created #17667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sgiehl @Findus23 @diosmosis