PathOfBuilding
PathOfBuilding copied to clipboard
PathOfBuildingCommunity
Improvements to testing tooling
Description of the problem being solved:
- Add an updated docker container with the Dockerfile available as part of the code base and emmylua support for debugging.
- Adds a github action for building and publishing this docker image under the POB org.
- Applies the image to tests for standardization
- Reworks the
test.ymlaction to better support test builds.- Uses github actions cache to speed up comparison
- Build diff should only ever fail on breaking changes (crashes)
- Adds some information to
CONTRIBUTING.mdregarding debugging of tests.- Also updates an outdated line regarding
ModCache.lua
- Also updates an outdated line regarding
- Comments out debug print of missing minion skills to clean up the log slightly.
To simplify the logic i've used my name in in place of what should be pathofbuildingcommunity so that will need to be fixed upon merge as i'd like to keep it this way for testing. Getting the org name in lowercase especially in actions proved to be more complicated than i'd like it to be.
we need to create the /workdir directory
Did you just use docker compose up to run the tests? It should create workdir and cache directories on mount:
https://github.com/PathOfBuildingCommunity/PathOfBuilding/blob/61b3d1057265a4e1d99e3b3f310a1ca18eb8510b/docker-compose.yml#L31-L33
That umask illegal mode is odd.
The tests have always been much slower when running locally than the GH runner. Not sure why that is. Open to suggestions on how it could be improved. Not much has changed for the unit tests container though. It should use the same versions of lua and luajit though the versions of busted may be different as they're the latest ones in the new container.
Looks like it's also using my settings file for the previously loaded build. That won't matter on GH actions, but could be something that trips someone up locally.
Iirc that's what the old tests did so i didn't change it. Could be something to improve.
EDIT: Freshly pulled git clone https://github.com/Paliak/PathOfBuilding.git on a fresh vm:
Should magiclines be magicLines?
That's what the person from Stack overflow called it so i kept it like that. Though 'splitLines' would probably be a better name.
I'm wondering if there's a good way to turn that func into a generator.
~~Artefact generation is broken. For some reason new lines are mangled in the GitHub action and docker logs are missing. Marking as draft as this will likely take a while to figure out.~~
Seems like the fix was easier than i thought. The diff still seems to display moved elements. Probably just need to write a custom diff tool for this.
The values of the output are now sorted in the xml before being passed into xmllint to prevent them from jumping around. I've thought about writing a custom diff tool but honestly i'm not sure what kind of diff output would be most useful.
TODO:
- Add caching to prevent hammering build hosts
- add automatic build list update logic using build api from https://github.com/PathOfBuildingCommunity/PathOfBuilding/pull/7389
- Make cache from gh actions available to pull to speed up local test runs
- Add logic for measuring difference in build calculation speed (warn if some build takes X% longer to computed compared to base branch)
- Add arbitrary branch diff. Remove hard coded branch origin/dev as it may not exist on local runs.
- Misc optimizations. Better docker build cache solution. Smarter git fetch behavior.
Setting as ready for review again. Implemented ability to choose source the reference branches. Implemented better build xml caching so as not to hammer build providers. Implemented warnings when a specific build takes longer than 10% to compute.
Only concern is the dawidd6/action-download-artifact action: https://www.legitsecurity.com/blog/github-actions-that-open-the-door-to-cicd-pipeline-attacks
I don't think the attack vector explained in the post applies here as the files downloaded from the artifact are never executed and are always extracted to dedicated directories or directories not meant to contain any executable code. Nevertheless i would like someone to triple check that there isn't some kind of a way to combine this with a zip-slip style vulnerability (those were found before in the used zip lib https://security.snyk.io/package/npm/adm-zip) to get code exec or if there is a better way of doing this (thought of manually using the github api but not sure if that's a good idea).
If you didn't want to use that other action to download artifacts, you could instead set the cache key to today's date, skipping the build list update on a cache hit and refreshing the list on a cache miss (AKA a new day has started). Wouldn't even have to update builds on a cron anymore either, just would update the list whenever needed.
@Wires77 I wanted it to update with cron as i'm worried there may be long periods of time without pull requests. Github only stores cache for 7 days and there is no way to alter that so if there happens to be a period of time longer than 7 days without any pull requests, the list would be lost and it would fall back to the static one. Which would bring the stale build list problem back.
@Wires77 I wanted it to update with cron as i'm worried there may be long periods of time without pull requests. Github only stores cache for 7 days and there is no way to alter that so if there happens to be a period of time longer than 7 days without any pull requests, the list would be lost and it would fall back to the static one. Which would bring the stale build list problem back.
Why would it fall back to the static one? The cache hit would fail if a test is run after 7 days, and then would run the step that downloads a new build list. This would occur on PRs, not just the base repo. Correct me if I'm misunderstanding the current workflow
@Wires77 I wanted it to update with cron as i'm worried there may be long periods of time without pull requests. Github only stores cache for 7 days and there is no way to alter that so if there happens to be a period of time longer than 7 days without any pull requests, the list would be lost and it would fall back to the static one. Which would bring the stale build list problem back.
Why would it fall back to the static one? The cache hit would fail if a test is run after 7 days, and then would run the step that downloads a new build list. This would occur on PRs, not just the base repo. Correct me if I'm misunderstanding the current workflow
Problem is the build list update action is limited by the number of builds returned by the api. Which currently is like 3-5 new ones per update so it pads it with the static list to ensure a good sample size.
Right now the entire build list update logic is based on accumulating the 3-5 new builds returned by the api everyday and rotating out old ones to keep the list fresh. Since the list didn't have enough time to accumulate the new builds i pad it with the static list to ensure a decent sample size.
If i only updated on cache miss i would basically always run static list + a few new builds which would bring back the stale builds problem.
If i had an api that just spat out x amount of build xmls then that would be ideal and would greatly simplify the logic but right now i'm trying to work with what i have access to while not hammering the build providers with crawling/re-downloading the builds.
I see, I was misunderstanding. Thought the downloaded builds were just overwriting things each time, not appending to the existing list. Build artifacts feel like a weird way to store this info to me, would a different repo that the action can push to make any more sense? Then we could put the cron job on that one and these actions just pull from there. I'm just spitballing ideas, this seems fine to me if a new repo seems overengineered :)
Build artifacts feel like a weird way to store this info to me, would a different repo that the action can push to make any more sense?
Don't know about more sense but i could make it work that way sure. Though handing this pr off is difficult as is as the sources will need to be changed from my repos to the POB org. Making a new repo to store the build list would fix a few problems such as running tests locally requiring a 3rd party service to download latest build list and using the 3rd party action for the artifact download. Though it would require more effort on your side to integrate as a new repo would need to be created the and secrets setup so that the action can push to the other repo.
Ideally i would want to just use an api that could get me already decoded xml builds since that would simplify a ton a things here but yeah.