-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add test workflow #60
Conversation
This is a great piece of work! The workflow looks a lot better and more nuanced than the initial one that I created and I think it can act as a great replacement! Please note that we're also in the process of moving reference projects to the unity-actions repository. This might mean that we will be using dispatch methods to run tests between repositories. I also wanted to say great job on https://github.com/mob-sakai/unity-changeset Get back to you soon. |
.github/workflows/test.yml
Outdated
on: | ||
workflow_dispatch: | ||
push: | ||
paths: | ||
- "**/Dockerfile" | ||
- .github/workflows/test.yml | ||
- reference-project-test/* | ||
pull_request: | ||
types: | ||
- "opened" | ||
- "synchronize" | ||
paths: | ||
- "**/Dockerfile" | ||
- .github/workflows/test.yml | ||
- reference-project-test/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale about becoming so specific about the triggers?
For example; why not simply trigger on all default PR events?
on:
pull_request: {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale about becoming so specific about the triggers?
This is to prevent unnecessary testing due to changes in files that are not related to building the image (e.g. changing only Readme.md
).
The test (48 jobs) takes about 1 h.
No problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say lets keep the paths, but remove the specific types. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workflow_dispatch
and push
may be redundant.
(They support the following)
- Trigger from other workflows or manually
- When a change is pushed directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say lets keep the paths, but remove the specific types. What do you think?
OK, I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep push
and workflow_dispatch
.
These will work in the forked repository, so we can test before creating a PR.
# For 'linux-il2cpp' module, switch the script backend to 'IL2CPP' | ||
if [ "${{ matrix.module }}" = "linux-il2cpp" ] ; then | ||
mv -f reference-project-test/ProjectSettings/ProjectSettings_il2cpp.asset reference-project-test/ProjectSettings/ProjectSettings.asset | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that altering the project will make the branch "dirty", meaning that builder will reject it unless "allowDirty" is passed. Maybe for testing purposes that would be ok.
Compared the changes between the il2cpp vs default project settings - note to self:
@GabLeRoux this might be a nice way to reduce the amount of test projects; simply swap some settings only.
@mob-sakai do you think unity-builder should differentiate between these target platforms? (And would it remove the need for this custom setup part?
- module: base
platform: StandaloneLinux64
- module: linux-il2cpp
platform: StandaloneLinux64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webbertakken
In unity-builder
and unity-test-runner
, should we distinguish between the "StandaloneLinux64-Mono" platform and the "StandaloneLinux64-IL2CPP" platform?
Or do you mean to support the base
and linux-il2cpp
platforms in unity-builder
and unity-test-runner
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the way we implemented it now is that StandaloneLinux64 (which doesn't have il2cpp until 2019.3) defaults to base and uses il2cpp from 2019.3 upwards.
Base is just a fallback because il2cpp doesn't exist, and we assumed that il2cpp would be the default/desired target platform for linux (and if not, that non-il2cpp would also be possible to build with il2cpp image).
I'm not sure what's best here. Suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I understand what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Suggestion:
- run: |
# For 2019.3 or later, non-il2cpp would also be possible to build with il2cpp image.
if [ `echo "${{ matrix.version }}" | grep -v '\(2018|2019.1|2019.2\)'` ] && [ "${{ matrix.module }}" = 'base' ] ; then
echo "MODULE=linux-il2cpp" >> $GITHUB_ENV
else
echo "MODULE=${{ matrix.module }}" >> $GITHUB_ENV
fi
###########################
# Build editor #
###########################
- name: Build
# if: steps.build-1.outcome == 'failure'
uses: docker/build-push-action@v2
id: build-1
continue-on-error: true
timeout-minutes: 40
with:
context: .
file: editor/Dockerfile
build-args: |
baseImage=base:dev
hubImage=hub:dev
version=${{ matrix.version }}
changeSet=${{ env.CHANGESET }}
module=${{ env.MODULE }}
tags: editor:dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, unity-test-runner
seems to use unityci/editor:<unity-version>-base-0
for testing.
https://github.com/game-ci/unity-test-runner/blob/main/src/model/image-tag.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I like your suggestion, lets go for that!
BTW, unity-test-runner seems to use unityci/editor:-base-0 for testing.
Yes, this is simply a result of our iterative approach. It should be able to test for all platforms to test platform-specific code paths. There's this issue that talks about custom image, but ideally we should also be able to use unity-test-runner in the same way as unity-builder, and specify targetPlatform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Great work! |
@webbertakken |
Added and reran the workflow, seems like there's still a discrepancy in activation for some versions, but the majority seems to work perfectly! Thanks a lot for your contribution. |
@webbertakken
|
The test appears to be successful, but the license activation fails. |
Right my bad, I thought that was the correct license I had lying around in my downloads folder. Updated and running again. |
Changes
unity-changeset list --versions --latest-patch --json --min 2018.3
to get the latest patch versions for each minor version.--min <version>
option.game-ci/unity-builder@main
andgame-ci/unity-test-runner@main
.reference-project-test
), it will automatically test.2019.1.x
,webgl
,linux-il2cpp
as reported in other issuesChecklist
NOTE
UNITY_LICENSE
that contains of an Unity license file (*.ulf).