matomo icon indicating copy to clipboard operation
matomo copied to clipboard

[Github Action] Test 4.x branch

Open peterhashair opened this issue 2 years ago • 23 comments

Description:

Fork https://github.com/matomo-org/matomo/pull/18290 update github-action branch has some unnecessary UI updates Test 4.x branch, not sure why but seems like php8.0 MySQLi driver has a poor performance here

UI viewer test: Because the silex is out of sup. Here is the Symfony version of it, but at the moment only tested the GitHub action part not fully tested Travis. https://github.com/matomo-org/ui-tests-viewer/pull/40

YML, PDF: when tests failed, the system will upload the artifact as zip and stay on Github for 5 days

Once this is merged to 4.x-dev, we can use the below to wait for the other process finished. Only works on default branch

  workflow_run:
   workflows: [ Build Vue files,PHPCS check ]
   types:
     - completed

Performance

It seems like the MYSQLI is twice slower as the PDO driver, not sure why.

Review

peterhashair avatar Jan 23 '22 22:01 peterhashair

@sgiehl revert some unnecessary code form change. that should make review easier.

peterhashair avatar Feb 08 '22 03:02 peterhashair

advantage:

  • The last commit will be canceled the preview build.
  • It seems like Github action overall will be 2-10 mins quicker.

disadvantage:

  • Entire Pipeline is running as a whole, you can not rerun a single job.
  • It seems php8.0 + MySQLI driver is very slow. not sure why could be sth to do with MariaDB.
  • Github Action is free on the public repo, but the limit is 20 concurrency jobs per repo. Meaning the max pipeline at the same time are 3 PR requests. Fork will remove the limit if we want to go this path.
  • The main test should wait for VUE/PHPCS build finished, then continue. But that option only works on the default branch, which is 4.x-dev. Or we could merge VUE, PHPCS build and test to one pipeline as a step

Push Live Require. Just a list for this to progress.

  • [ ] [Review] Code review on this branch.

  • [ ] [Decision] do we need Travis and Github action running at the same time, or completely remove Travis Suggestion: We keep both running for at least 1 week.

  • [ ] [Review] Submodule PR review. Submodule target change to 4. x-dev

  • [ ] [Decision] do we allow UI upload for public fork repo. Do we consider displaying the failed UI HTML inside GitHub action?

    • the benefit of fork repo is not limited by concurrency. Suggest: we move screen compared to GitHub action, instead of using https://builds-artifacts.matomo.org/
  • [ ] Doc] Documentation for development guild

  • [ ] [Doc] Documentation for internal staff use.

peterhashair avatar Feb 10 '22 02:02 peterhashair

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

github-actions[bot] avatar Feb 21 '22 01:02 github-actions[bot]

@tsteur somehow this test on MySQL running really slow, but MariaDB is a little bit quick. Any idea? PHP (SystemTestsCore, 8.0, MYSQLI)

peterhashair avatar Mar 06 '22 20:03 peterhashair

@peterhashair no idea why but you could either try setting testdox=true in the phpunit.xml.dist and then monitor if it's one particular test that takes a while. Or I believe there is also another feature maybe to print how long each test took to execute in phpunit. This would then help you figure out if there's one or few tests that are slow or all of them.

I just checked and testdox gave me this output for example: image

tsteur avatar Mar 07 '22 03:03 tsteur

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

github-actions[bot] avatar Mar 17 '22 01:03 github-actions[bot]

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

github-actions[bot] avatar Mar 24 '22 02:03 github-actions[bot]

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

github-actions[bot] avatar Apr 01 '22 02:04 github-actions[bot]

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

github-actions[bot] avatar Apr 09 '22 01:04 github-actions[bot]

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

github-actions[bot] avatar Apr 20 '22 02:04 github-actions[bot]

@sgiehl move all tests to here https://github.com/matomo-org/github-action-tests makes more sense, but needs a little work, start working on it.

peterhashair avatar May 06 '22 01:05 peterhashair

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

github-actions[bot] avatar Jun 10 '22 02:06 github-actions[bot]

There is one random PHP test that failed remain, JS test won't end.

peterhashair avatar Jul 06 '22 03:07 peterhashair

@peterhashair we would want to use php8.1, if not already then soon. Be worth spending an hour or so understanding what's happening in PHP8.1

justinvelluppillai avatar Aug 04 '22 04:08 justinvelluppillai

@sgiehl yes, should use https://github.com/matomo-org/github-action-tests, just trying to pass all the tests first, then I can covert them to a package.

peterhashair avatar Aug 09 '22 02:08 peterhashair

@sgiehl yes, should use https://github.com/matomo-org/github-action-tests, just trying to pass all the tests first, then I can covert them to a package.

So if you know that your work isn't finished yet, why exactly are you requesting a review?? I'm happy to also have a first look in between to e.g. check if something goes into the right direction. But in that case you need to leave a comment so I'm aware of the current state and what exactly to look at. Otherwise this is a total waste of my time.

sgiehl avatar Aug 09 '22 07:08 sgiehl

@sgiehl sorry about that, just trying to check, if all the test performance is correctly, before converting to package.

peterhashair avatar Aug 10 '22 00:08 peterhashair

Updates, group them into a GitHub package now. Still, some tests fail to try to fix. Seems like GitHub Action provides only run failed jobs options, now all the tests are in one file.

peterhashair avatar Aug 15 '22 22:08 peterhashair

@sgiehl Group them into the package, maybe can give a init review, I believe is just the angular keep failed.

peterhashair avatar Aug 16 '22 05:08 peterhashair

@sgiehl convert the mysql, redis etc to the package, now should be a simple setup. Convert one plugin as a test, here https://github.com/matomo-org/plugin-TasksTimetable/pull/29

peterhashair avatar Aug 23 '22 02:08 peterhashair

@sgiehl fixed the error, but I am confused about that one. Do you mind giving an example?

I don't think, that our tests should in the future only run for pull requests. They should also run for every commit on any base branch (4.x-dev, 5.x-dev) and maybe even manually.

peterhashair avatar Sep 08 '22 03:09 peterhashair

I don't think, that our tests should in the future only run for pull requests. They should also run for every commit on any base branch (4.x-dev, 5.x-dev) and maybe even manually.

We need to make sure that not only pull request have test running, but also our main branches after each commit/merge. Otherwise we can't be sure that tests would still be passing after something is merged.

Guess it would need to be something like

on:
  pull_request:
    types: [opened, synchronize]
  push:
    branches:
      - '**.x-dev'
      - 'next_release'

sgiehl avatar Sep 08 '22 03:09 sgiehl

@justinvelluppillai both Travis and GitHub action is enabled, still, some random PHP failed as expected, also for the UI test, I think we can only use one instead of the other, it seems like the 2 system has some IP or fonts difference, make the UI unique.

The other funny part seems like the php8.1 + MySQLI driver is significantly slower than php7.2 + PDO driver, not too sure why.

peterhashair avatar Sep 20 '22 02:09 peterhashair

As we discussed, we can merge with Travis, temp disables GitHub action UI tests, once we confirm the GitHub action is solid, then we enable the UI tests again.

peterhashair avatar Sep 28 '22 01:09 peterhashair

That sounds good to me @peterhashair as long as we have no security issues and can run them concurrently.

justinvelluppillai avatar Sep 28 '22 01:09 justinvelluppillai

@justinvelluppillai I don't think there is a security issue there, but if someone can review this would be good, still a coupe random fails, but I think that's expected.

peterhashair avatar Sep 28 '22 01:09 peterhashair

@bx80 are you at all familiar with this work to be able to give it a final review?

justinvelluppillai avatar Sep 28 '22 04:09 justinvelluppillai

@justinvelluppillai, @peterhashair I can take a look :+1:

bx80 avatar Sep 28 '22 21:09 bx80

@bx80 all the Gtihub Action codes tests code actually are here: https://github.com/matomo-org/github-action-tests.

peterhashair avatar Sep 28 '22 21:09 peterhashair

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

github-actions[bot] avatar Oct 06 '22 02:10 github-actions[bot]

@peterhashair For the action repo it might be helpful to create a temporary branch and create a PR from main against that branch. It's a lot easier to do a review, if there is a PR that can be commented.

sgiehl avatar Oct 10 '22 12:10 sgiehl

@peterhashair For the action repo it might be helpful to create a temporary branch and create a PR from main against that branch. It's a lot easier to do a review, if there is a PR that can be commented.

refer https://github.com/matomo-org/matomo/pull/19843

peterhashair avatar Oct 11 '22 02:10 peterhashair

@peterhashair For the action repo it might be helpful to create a temporary branch and create a PR from main against that branch. It's a lot easier to do a review, if there is a PR that can be commented.

refer #19843

I guess you misunderstood that. I meant creating a PR on that repo: https://github.com/matomo-org/github-action-tests. That repo contains the most relevant code and needs a detailed review...

sgiehl avatar Oct 11 '22 15:10 sgiehl

This PR needs to be reviewed together with https://github.com/matomo-org/github-action-tests/pull/2 and one of the plugin PRs like https://github.com/matomo-org/plugin-TasksTimetable/pull/29 Only if all work smoothly together this one can be merged.

sgiehl avatar Oct 13 '22 11:10 sgiehl

@sgiehl do we need anything to get this merged?

peterhashair avatar Oct 26 '22 02:10 peterhashair

As I will be away the next days here a small update on the current state of the github test action. I have refactored and simplified various parts of the action and added some missing parts that are required for running tests for plugins. In core, as well as for public and private plugins the action seems to run more or less correctly. There are still a couple of failures that I need to investigate. Once everything works as expected I will start implementing a command to generate the action files for plugins. So we can finally start rolling out the action to all repos. Will continue to work on it when I'm back.

sgiehl avatar Dec 23 '22 18:12 sgiehl

Build finally succeeded. Even though there are still some randomly failing tests like before.

sgiehl avatar Jan 31 '23 13:01 sgiehl