PR test runner

[saurtron]

• based on previous PR by @salinecitrine.
  • see there for further info and discussion

Work done

• base github actions and tooling by @salinecitrine
• mocha reporting:
  • shows success, failed, skipped tests.
• workflows:
  • make the deploy jobs depend on successful tests.
  • at request of @p2004a joined everything into one workflow file.
    • has three internal stages: run_test, process_test, deploy
• add a special test scanning infolog.txt for errors.
• update to run on Supreme Isthmus v1.8

More details

for convenience, from #2626:

This uses https://github.com/EnricoMi/publish-unit-test-result-action to publish test results for PRs, including forks. It uses roughly the setup described here to do so. This means there is an action that runs on the PR branch that runs the tests, and an action that runs on the master branch that publishes the results.

Remarks

• This is the result on an epic journey by @salinecitrine, he created the test runner, created the github actions. An incredible amount of work, so would like to thank him and send ❤️.
• The test runner shows a bot comment like this at PRs, this can be a bit verbose and could get old fast. It can be disabled.
• Does release deploy only if the tests pass, so once this is in can't deploy automatically if the tests fail.
  • Seems there's a secondary publish method running on cron somewhere, so if we want to use only deploy if tests work will need to avoid using that, unless there's a situation that requires it.
  • Should be best... but ninja commits (without PR and previous checking of tests working) can be bad.
• To block merging when the checks don't pass, I believe this needs to be configured at github project.
• The infolog.txt test goes further than normal tests, since it scans any infolog.txt occurring during running of tests and initialization. It should catch weird situations.
• Due to the (current) headless nature of tests, they can fail in situations where normally code wouldn't fail.
  • For widgets and gadgets hard requiring opengl, best is to add depends: {"gl"} at info
  • Others will benefit from clear separation of draw and logic so tests can be run even when drawing part isn't working
  • Maybe later we find a way to run headless but without the headless engine, that would allow more things to work fine and be tested automatically.

[saurtron]

Ok looks like need to address https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.18.0/README.md#support-fork-repositories-and-dependabot-branches before the tests will run on PRs from separate repos. Will check this soon.

Anyways can use review already.

[salinecitrine]

Ok looks like need to address https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.18.0/README.md#support-fork-repositories-and-dependabot-branches before the tests will run on PRs from separate repos. Will check this soon.

Anyways can use review already.

Haven't had much of a chance to review things (sorry!), but FYI the previous PR (#2626) was set up to handle fork PRs as described in that link. Could probably steal the setup from there at least as inspiration.

[saurtron]

Haven't had much of a chance to review things (sorry!), but FYI the previous PR (#2626) was set up to handle fork PRs as described in that link. Could probably steal the setup from there at least as inspiration.

Yeah I totally based on that one, I think I removed smth related when merging workflows into one file tho. Thx for the tip!

[saurtron]

Could probably steal the setup from there at least as inspiration.

Ok I think it's working now but the secondary workflows need to be in master for them to work. Difficult to tell tbh XD. I did try at saurtron#9 and still seems to work.

Anyways I'll still tweak a bit since some things need to be changed when in 2 files and not sure everything is tuned right now. I tried before with 3 separate workflows (like yours + deploy), also with 2, so I'll review.

[p2004a]

I removed smth related when merging workflows into one file tho

bd43a2e, 139 more lines to read and maintain. Why?

[saurtron]

bd43a2e, 139 more lines to read and maintain. Why?

Seems we overlooked this: https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.18.0/README.md#support-fork-repositories-and-dependabot-branches

Its just ~40 lines more btw, anyways doesn't seem to be much we can do about it, can't fix without some extra lines, sorry.

[saurtron]

bd43a2e, 139 more lines to read and maintain. Why?

A bit more background, after having checked the issue, that required those changes:

This seems to be a security mechanism to avoid getting hacked through github actions, check this: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/.

These parts are the most interesting:

"Since, by definition, a PR supplies code to any build or test logic in place for your project, attackers can achieve arbitrary code execution in a workflow runner operating on a malicious PR in a variety of ways."

"Due to the dangers inherent to automatic processing of PRs, GitHub’s standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository."

[saurtron]

This seems to be a security mechanism

Also, having read all that, I'd say the original three workflow way would be the most secure in order to avoid getting hacked by https://github.com/EnricoMi/publish-unit-test-result-action itself, although we could avoid that by having a private copy of that, or pinning some specific commit maybe. Not totally sure though, would have to read more about the permissions... also I think disabling comments for that action makes it not require write access to the repo.

[S3KCentrifugal]

To keep code pretty, maybe add a linter? (I didn't see one being used)
https://github.com/marketplace/actions/luacheck

Just found this action available, but I am sure there are others.

[oopsbagel]

To keep code pretty, maybe add a linter? (I didn't see one being used) https://github.com/marketplace/actions/luacheck

Just found this action available, but I am sure there are others.

If you're going to add a lua linter, I'm personally fond of selene, a more active project -- there's also a github action.

[p2004a]

@saurtron what is the status of this? It's still in draft mode

[saurtron]

@saurtron what is the status of this? It's still in draft mode

I think I'm going to split the run_tests_and_deploy (note: already did it, check below comment) out for now, so quick_deploy won't depend on tests for now, I'll also check all tests still pass properly.

Then I will make an additional PR joining them again, to apply when we're sure it's not going to be disruptive. I think it's more sensible than trying to do everything in one go. It's great having tests blocking merge, or deploy if something broken is commited, but doesn't need to go all in in one go.

Let me know if you don't like this approach, and give me 1-2 days and I'll prepare it so it can be merged already.

[saurtron]

Let me know if you don't like this approach, and give me 1-2 days and I'll prepare it so it can be merged already.

Ok, did it already, also created #4737 as "part 2" so in the end is the same we had before and already reviewed.

Now I need to check some master test failures on headless before this can be merged (ie, to remove the DRAFT status).

[saurtron]

Found a number of issues with tests on current master, fixes here:

• Skip some modules on headless. #4744
• Introduce new dbg_test_headless_overrides widget #4743
• ecostats: Fix bad check for maxMetal > 0. #4742
• Fix loading widgets with locals access enabled. #4741

Once those are in this can be "undrafted".

[p2004a]

LGTM. Let's merge after those are fixed and see how it behaves.

[saurtron]

Ok, the mentioned changes are in, so this is ready for final review.

I have it merged at my repo's master, so if you want to see it work I think you can PR there. (probably a good idea so we see it works with external PR)

I did a PR myself to myself, with the cmd-extension-mechanism as an example. saurtron#15 (you can see the test results there)

[WatchTheFort]

Does this supersede #2626? Should #2626 be closed in favour of this one?

[saurtron]

Does this supersede #2626? Should #2626 be closed in favour of this one?

yeah I think it can be closed, its built on his with some changes and then a follow up PR #4737 for when/if we want to make deploy depend on successful tests

[p2004a]

@saurtron per saurtron#16 looks like some test is currently failing on master.

Also can you explain the pending tests? Why pending is not failing?

[WatchTheFort]

@saurtron What is the status of this? Is it ready to merge?

[saurtron]

@saurtron What is the status of this? Is it ready to merge?

Need to wait until engine 2025.04.10+ before testrunner is usable since they don't work with 2025.04.08/09 due to too strict lua security policy.

After that will have to review and fix any current errors in master again. Sorry about that, its just merging this requires tests to be passing at the time and its kind of a moving target for now.

Anyways looking close when the bar engine is updated to current latest stable 2025.04.10 or more recent.

[NortySpock]

@saurtron Looks like http://github.com/beyond-all-reason/Beyond-All-Reason/pull/5702 merged recently... does that unblock this PR?

(Call it heresy if you will, but would commenting out most of the tests help accelerate the process, and then a follow-on PR performs the test fixes, post-merge?)

(Just curious because I feel automated CI testing would be nice to see in this project.)

[WatchTheFort]

Does this PR supersede #2626? Both PRs add the same set of Github action files.

[p2004a]

Anyways looking close when the bar engine is updated to current latest stable 2025.04.10 or more recent.

That precondition did not happen, bar is still on engine 2025.04.08.

[p2004a]

Does this PR supersede #2626? Both PRs add the same set of Github action files.

First line of this PR:

based on previous PR by salinecitrine. see there for further info and discussion

[NortySpock]

Recommend closing this PR as the now-merged #5874 (which is just a rebase of this PR with some tests skipped) covers this functionality.

Thanks for all the hard work you put in getting this PR working @saurtron . I stand upon the shoulders of a giant.