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

[Vue] add test to make sure umd module files are production builds #18076

Closed
wants to merge 2 commits into from

Conversation

diosmosis
Copy link
Member

Description:

To catch when someone pushes a development build of a UMD module (as I did in the alert PR).

Review

@diosmosis diosmosis added 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 labels Sep 29, 2021
@diosmosis diosmosis added this to the 4.6.0 milestone Sep 29, 2021
@sgiehl
Copy link
Member

sgiehl commented Sep 29, 2021

@diosmosis would it make sense to also have a test that checks that the built vue files are up to date? Wondering if it otherwise could happen that someone changes a vue component but forgets to update or push the umd files. 🤔

@diosmosis
Copy link
Member Author

@sgiehl That sounds like a good idea... I guess it could be done by checking if the diff w/ 4.x-dev has changes to vue/src files, and making sure there are also changes to vue/dist. But that wouldn't be foolproof since a dev could forget to compile after making an additional source code change.

Alternatively we could modify the travis-scripts to run the build command (after doing an npm install) and checking that nothing was modified. What do you think?

@sgiehl
Copy link
Member

sgiehl commented Oct 1, 2021

Not sure if adding that to travis is a good option. Personally I would prefer moving such stuff to a github action. Similar to the PHPCS the action could simply run building the vue js files and check if there would be any changes available. That way we could even enforce the check and a PR couldn't be merged anymore if the files aren't up to date.

@diosmosis
Copy link
Member Author

@sgiehl ok, i'll look into using a github action.

@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Oct 10, 2021
@diosmosis diosmosis added Do not close PRs with this label won't be marked as stale by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Oct 10, 2021
@tsteur
Copy link
Member

tsteur commented Oct 11, 2021

@diosmosis can we merge this already or do we need to wait for the github action?

@diosmosis
Copy link
Member Author

@tsteur the github action will prevent more issues so I'll still look into that.

@diosmosis
Copy link
Member Author

Closing in favor of #18080 which will rebuild vue files automatically.

@diosmosis diosmosis closed this Oct 12, 2021
@diosmosis diosmosis deleted the vue-production-build-test branch October 12, 2021 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Do not close PRs with this label won't be marked as stale by the Close Stale Issues action Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants