Add Feature to test Unity Packages

[trudeaua21]

Changes

• closes #71
• Makes the husky pre-commit hook executable
• Adds a new argument to the action named packageMode, which is false by default. If true, then the action tests a Unity Package rather than a Unity Project
• In package mode, all functionality is the same for testing packages as for testing project, except for
  • caching, which isn't available
  • an explicit Unity version is required
  • in package mode, the CLI utility jq is used, which means that any docker image used to run the action in package mode must be compatible with apt-get
  • tests are run by creating an empty Unity project at runtime, adding the package to that project, and testing the project.
• A test Unity package has been added to the repo, as well as several workflow items in main.yml to test the action's usage on the package.

Areas for Improvement

Here's some clear areas for improvement with the feature that I haven't yet implemented. If needed, I will try implementing these before the PR is merged, but if not, then I'll fill out a new issue for these improvements.

• Caching may be able to be implemented in package mode by caching the empty project that gets created every time the tests run. The project would need to be cached before the package is added to its manifest and testables.
• Having a unity version of auto shouldn't be too hard to implement - I'm fairly sure it can just be taken from the package's package.json. The main reason it's not part of this PR is just because I was focusing on getting eyes on the MVP first.

Checklist

• [x] Read the contribution guide and accept the code of conduct
• [x] Readme (updated or not needed)
• [x] Tests (added, updated or not needed)

[github-actions[bot]]

[GabLeRoux]

Nice one! Thanks for the contribution :D

in package mode, the CLI utility jq is used, which means that any docker image used to run the action in package mode must be compatible with apt-get

We now have support for windows based docker images so another area of improvement might be to add a condition to get this working for windows based images as well, but that can definitely be in a next step. 👍

[trudeaua21]

By the way, I noticed the pre-commit hook wasn't working for me a few commits back. I had just been manually doing the pre-commit steps on my own for a little while, but with this recent batch of changes, I decided to dig a bit deeper and found that the problem was just that the file for the pre-commit hook wasn't executable.

I updated the file to be executable and the hook started working for me again, so I figured I was good to commit that change - if that's not right feel free to call it out!

[webbertakken]

By the way, I noticed the pre-commit hook wasn't working ... I updated the file to be executable and the hook started working for me again, so I figured I was good to commit that change

Great, I've seen the hooks not working for more people indeed. Thank you very much. We should probably apply the same fix to all our repos.

[andywiecko]

Hey, great work!

I just want to share with you my idea. I apologize if I should post this as a separate issue, but I thought it could be easily implemented in the following PR if my thinking makes any sense.

---

Currently, custom Unity Packages do not support the "git dependencies" (and I read somewhere at unity forum that it will probably never be supported). In the case when one has a package that depends on other custom packages, the current solution is no longer valid, unless packages are available in some registry. Consider the example package.json, where "packageA" and "packageB" are custom unity packages:

{
    "name": "package",
    "version": "0.1.0",
    
    ...

    "dependencies": {
        "packageA": "1.1.0",
        "packageB": "1.2.0"
    }
}

Then one has to add them as a dependency at project manifest.json:

{
  "dependencies": {
     "packageA": "git+https://github.com/user/packageA#1.1.0",
     "packageB": "git+https://github.com/user/packageB#1.2.0",
     
    ...
  }
}

I propose to add a parameter, like additionalDependencies, then the job configuration for my example can look like following

   // ...

    - uses: game-ci/unity-test-runner
        id: tests
        with:
          projectPath: ${{ matrix.projectPath }}
          unityVersion: ${{ matrix.unityVersion }}
          testMode: ${{ matrix.testMode }}
          artifactsPath: ${{ matrix.testMode }}-artifacts
          packageMode: true
          additionalDependencies:
             -  "packageA": "git+https://github.com/user/packageA#1.1.0"
             -  "packageB": "git+https://github.com/user/packageB#1.2.0"
             
   // ...

Best, Andrzej

[trudeaua21]

@andywiecko Great catch! I think that might end up being a little too complex for me to take on right at this moment, so if the maintainers are alright with it I think I'll just note that packages with dependencies like you've described aren't able to be tested yet, and make a new issue to address that.

[JesseTG]

Thankfully with the prevalence and simplicity of OpenUPM there's a lot less need for git-specified dependencies.

[trudeaua21]

TL;DR

Packages that depend on external registries or other package dependency approaches probably don't work with the current package support of the runner from this PR. I think it's definitely feasible to add support for those, but unsure if it should come with this body of work or not.

Long version

Honestly, in the current state of this work, I don't think it's even possible to test packages that depend on any external registry. I would hope that the solution is as simple as just tossing the scoped registries' information into the manifest of the generated Unity project we use for testing manually (similarly to how adding the package under test to the manifest manually currently):

{
   // HOPEFULLY WE COULD JUST ADD THE REGISTRY INFO HERE
    "scopedRegistries": [
        {
            "name": "General",
            "url": "https://example.com/registry",
            "scopes": [
                "com.example", "com.example.tools.physics"
            ]
        },
        {
            "name": "Tools",
            "url": "https://mycompany.example.com/tools-registry",
            "scopes": [
                "com.example.mycompany.tools"
            ]
        }
    ],
    "dependencies": {
        "com.sample.project": "location"
    }
}

I would guess that packages that depend on other packages in the official Unity registry will work fine with the runner in its current state (since I'm assuming that Unity's package manager will be able to resolve them without issue), but I will need to check that to make sure.

I think I would need to ask the maintainers their preference here, since there's two options:

1. After I finish troubleshooting the issues from those who tested this, we can merge this work with the functionality as-is, with an explanation in the documentation that packages with dependencies that are not from the Unity package registry are not supported. We could then open a new issue and begin working on adding support for those packages.
2. We keep this same PR open, and wait to merge it until after support for the packages described above are added

I personally prefer option (1), since this PR has been open for a while already and it would be nice to start on the other tasks with a clean slate, but I could definitely understand support for option (2) as well, as some people may not read the docs and try/fail to test their packages that aren't supported.

[webbertakken]

I think I would need to ask the maintainers their preference here, since there's two options

We mostly prefer small iterations where possible so if this is a chunk that works for your use case already and it doesn't create forward incompatibility we're good to merge it.

[JesseTG]

Is there any reason this PR wouldn't work with third-party (but still NPM-powered) scoped registries? I'd really like to use it for a test pipeline for a package I'm working on, but I can only do that if it can load dependencies from OpenUPM.

[trudeaua21]

Is there any reason this PR wouldn't work with third-party (but still NPM-powered) scoped registries? I'd really like to use it for a test pipeline for a package I'm working on, but I can only do that if it can load dependencies from OpenUPM.

@JesseTG There isn't a reason past that it will take more time to implement and then test. It seems like it would be simple to implement support for scoped registries, based on the fact that the package manifest for Unity Projects has scoped registry URLs present, indicating that everything Unity needs for scoped registries can be found in the package manifest.

That said, I believed this body of work would be simple as well, and while all things considered, it kind of was, it still has taken a good few months due to some unforseen complications 😅

I'll probably work on it after this gets merged, but the functionality could still take some time.

[saszer]

Can this also be extended with the feature to do builds with just packages as well in future?

Ps, being eyeing this, hope it gets merged soon. Extra features with package dependency compatibilities etc, can be a separate issue imo.

[andywiecko]

@saszer Good idea for the builds! I reported this in unity-builder repo: https://github.com/game-ci/unity-builder/issues/363

[JesseTG]

Is there any reason this PR wouldn't work with third-party (but still NPM-powered) scoped registries? I'd really like to use it for a test pipeline for a package I'm working on, but I can only do that if it can load dependencies from OpenUPM.

@JesseTG There isn't a reason past that it will take more time to implement and then test. It seems like it would be simple to implement support for scoped registries, based on the fact that the package manifest for Unity Projects has scoped registry URLs present, indicating that everything Unity needs for scoped registries can be found in the package manifest.

That said, I believed this body of work would be simple as well, and while all things considered, it kind of was, it still has taken a good few months due to some unforseen complications 😅

I'll probably work on it after this gets merged, but the functionality could still take some time.

Ah, I think I see the problem. package.json (the file you're extracting info from) doesn't generally care about particular registries, but manifest.json (the file you're creating) does. If scoped registries aren't available in either file, you'd need a third source.

But I think it would be easiest to require that packages tested with this action define the necessary scoped registries in package.json. Then you could just copy the scopedRegistries object and insert it directly into the generated manifest.json. What do you think?

[trudeaua21]

But I think it would be easiest to require that packages tested with this action define the necessary scoped registries in package.json. Then you could just copy the scopedRegistries object and insert it directly into the generated manifest.json. What do you think?

I actually didn't know that scoped registries could be defined in the package.json of a Unity package - do you have an example of this? Pretty much all I know about package.json structure just comes from this page (and the related docs), and I can't find an example of having scoped registries in package.json: https://docs.unity3d.com/Manual/upm-manifestPkg.html

Not to say I don't believe it's possible (lord knows there's some fun undocumented stuff with Unity packages), I just couldn't find it at a moment's glance. If it's possible, that would make implementation of this feature easier 😄

[JesseTG]

But I think it would be easiest to require that packages tested with this action define the necessary scoped registries in package.json. Then you could just copy the scopedRegistries object and insert it directly into the generated manifest.json. What do you think?

I actually didn't know that scoped registries could be defined in the package.json of a Unity package - do you have an example of this? Pretty much all I know about package.json structure just comes from this page (and the related docs), and I can't find an example of having scoped registries in package.json: https://docs.unity3d.com/Manual/upm-manifestPkg.html

Not to say I don't believe it's possible (lord knows there's some fun undocumented stuff with Unity packages), I just couldn't find it at a moment's glance. If it's possible, that would make implementation of this feature easier 😄

You can include pretty much any data you want in a package.json file. Unity won't necessarily use it...but you can. I think it's reasonable to require (for the purpose of using this Action) that a package.json list the scoped registries that the main manifest.json uses.

It would be an unusual use of the format, but it would be intuitive and within the schema's rules. The list of scoped registries in a package or project doesn't change too often, so I don't think it would be a burden to expect a package author to keep it in sync with a local manifest.json.

[trudeaua21]

Yeah, that seems like it would be feasible to implement in that way.

Just an update for anyone following the thread - here's what's going to happen before this gets merged:

1. My contribution which adds jq to the installed tools of the game-ci docker images, which the unity-test-runner leverages, is pretty much complete (hopefully - there's some failing checks but that could be a fluke, since they didn't fail on a different run of the test workflow). This will need to get merged before this feature is functional again. Done!
2. After that gets merged, I'm going to run a test to confirm that this feature works on a basic, barebones package (go to the issue for this feature if you'd like to see a package someone put together which will help test this)
3. After that, I'm going to add some tests to my own package that reference a dependency from the Unity package registry to confirm that Unity package registry dependencies work with this feature. If this fails, I'll have to re-evaluate completion time. I've confirmed that dependencies from the Unity Registry are properly resolved when the tests run, so all that's left is the other two steps!

After that, I feel like the first iteration of this feature is pretty much done! 😄

After this feature gets merged, provided someone else doesn't do it first, I'll make an issue or two to track the improvements to this feature (Windows image support, support for scoped registries, support for git dependencies, etc.)

Thanks to everyone for your help and patience 😄

[trudeaua21]

Per my comment on the issue, next monday I'll have the time to start working on this again!

It looks like there's another PR with some changes that will likely get merged before I finish this up, so I'll make sure to update this to ensure compatibility with the features from that PR.

[trudeaua21]

Alrighty, I feel like the first iteration of this feature is pretty much done! I've gone ahead and re-requested review.

Here's an example of the feature working on an existing package.

I'm not sure why the workflow is failing - I'm a bit stumped because the problem isn't happening on my fork.

My steps after this gets merged will be as follows:

1. Document possible extensions of this work/limitations of this work in a new issue or two.
2. Add some more in depth documentation for this new feature

Thanks for your patience everyone! 😄

[trudeaua21]

Actually, in case this does get reviewed, please wait to merge (assuming everything ends up looking good) until I implement the fix I've described on the issue for packages located on the repository root folder. Should be quick and easy

[trudeaua21]

BTW, I've been pretty busy the past couple weeks, and the rest of this week - not sure when I'll get around to addressing the feedback.

Also, there's going to be one more functionality tweak I'd like to add before this gets released - currently, the action fails if the directory to the package is the root directory of the repository. I've confirmed a workaround exists by making a copy of the root directory to some subdirectory before the tests run, but I need to add that to automatically happen within the action.

[webbertakken]

BTW, I've been pretty busy the past couple weeks, and the rest of this week - not sure when I'll get around to addressing the feedback.

No rush :)

Also, there's going to be one more functionality tweak I'd like to add before this gets released - currently, the action fails if the directory to the package is the root directory of the repository. I've confirmed a workaround exists by making a copy of the root directory to some subdirectory before the tests run, but I need to add that to automatically happen within the action.

Generally we support having projects in the root. It might be a bad idea to just copy the root directory, especially with regards to project size and file permissions. Do you know why it's failing exactly, when the project is in the root?

[trudeaua21]

Hey all - my day job has been pretty busy the last couple months and left me little time to dedicate to this. However, this Wednesday/Thursday I'll finally have some time to dig back into this and hopefully address all the feedback 🤞

[trudeaua21]

Generally we support having projects in the root. It might be a bad idea to just copy the root directory, especially with regards to project size and file permissions. Do you know why it's failing exactly, when the project is in the root?

Spent a good chunk of the day looking through the logs and trying out different things in an attempt to figure this out. I've eliminated a few possible conditions that I initially suspected were the problem, including:

• presence of .git, .github, and _activate-license files
• Permissions of the folder containing the package being tested or any of its files (not including files in any subdirectories)
• Permissions of the folder of the generated project or any of its files (not including files in any subdirectories)

The specific thing going wrong is that when the package being tested is the root of the repository , the test command fails. Roughly 20,000 lines of logs accompany the failure, but before those lines start this gets logged:

2022-07-01T01:50:11.0455269Z ###########################
2022-07-01T01:50:11.0455805Z #   Testing in playmode  #
2022-07-01T01:50:11.0456324Z ###########################
2022-07-01T01:50:11.0456608Z 
2022-07-01T01:51:06.3636835Z 
2022-07-01T01:51:06.3637523Z Aborting batchmode due to failure:
2022-07-01T01:51:06.3638242Z Scripts have compiler errors.

Another oddity I found - when the package folder IS the directory root and the action fails, there's many statements in the logs like these:

2022-07-01T01:51:06.9336866Z GUID [a6351153cf5e1af468cae5dbc72d1a04] for asset 'Packages/com.trudeaua.easyaccessibility/TempProject/Library/PackageCache/[email protected]/CHANGELOG.md.meta' conflicts with:
2022-07-01T01:51:06.9337280Z   'Packages/com.unity.testtools.codecoverage/CHANGELOG.md.meta'
2022-07-01T01:51:06.9337483Z Assigning a new guid.

None of these GUID conflicts are present in situations where the package folder is not the directory root, including cases where whatever workflow is calling the action moves the package folder out of the directory root.

Here's logs for a run of the action where everything works as intended: 1_tests.txt

Here's logs for a run of the action where the action fails due to the package folder being the directory root: 1_tests.txt

[trudeaua21]

Hey, so update on this.

I never was able to figure out why there was a problem with packages in root. I was able to verify that the problem could be worked around by moving the project out of root in a GitHub Workflow step before the test runner runs.

I've got to be completely honest here - given some recent decisions Unity has made as a company, I really can't justify continuing work on their platform right now to myself. In addition, those same decisions have completely taken the wind out of my sails on this work as well.

Aside from digging deeper into the issue with the package being in root (I think that debugging that problem would take a large chunk of my time, which I don't think would be manageable given my lack of motivation on Unity work right now), I'd be cool with doing whatever is needed to sort of wrap up work on this (like writing documentation, opening further issues, etc). Past that, I don't really see my self working on this feature.

I would also understand if the maintainers would rather not merge this feature as it is right now, as it might not really be considered "done" as it would require a workaround for some users.

More than anything, I'd like to really say thank you to the maintainers for your help and attention in developing this feature. You all rock and it's been a pleasure working with you on this.

I'm sorry if this makes any trouble for you all.

[webbertakken]

Completely understandable if your priorities are shifting.

To me the feature doesn't need to be 100% done as long as it covers at least the flow you were going for initially. Iterations are fine and other people would be able to build on top of your work.

If you're up for finalising this or as you say, rounding it up, then I think that'd be the most favourable option.

Thank you for your contributions as well!

[trudeaua21]

Sounds great, thanks for your understanding! Sometime in the next week or two I'll re-test that the package works for my initial use-case and add some documentation for the new features (and for the workaround for having packages in root).

[trudeaua21]

Hey all, see https://github.com/game-ci/unity-test-runner/issues/71#issuecomment-1502141778 for a quick update on this.

Essentially, I never really got around to wrapping this ticket up, but I'm planning on chipping away at it over the next few weekends to try to get it wrapped up.

Apologies that this hung around so long - hopefully I can get this off your plates!

[trudeaua21]

@davidmfinol @webbertakken Sorry for the ping, but I need your input on the questions at the bottom.

This is just about ready for review now - I've caught it up with the latest changes and updated the documentation in a couple of spots, as well as adding a check in the script which throws an error if it can't find the jq command (which could happen on a custom Docker container).

Here are the list of caveats/limitations of the feature as it stands right now:

• The test runner will fail if the directory of the package being tested is the same as the root directory of the repository.
• The test runner can only test packages on Linux runners - Windows runners are currently not supported.
• Packages with dependencies outside of the Unity Registry (for example, packages with Git Dependencies, Scoped registries, or having anything to do with UPM) have not been tested, so the test runner may or may not currently work with such packages.
• There is currently no caching set up for package mode.
• A unityVersion configuration of 'auto' cannot be used when testing a Unity package. The Unity version must be manually set in the action.
• If using the customImage parameter to use a custom Docker image to test the package, that image must have jq installed, or else the test runner will error. It is used to add the package being tested to a temporary Unity project's package manifest at runtime.

I just have a couple questions that I need answered in order to finish up now:

1. For all except the last item (regarding jq on custom images) of the list of limitations above, I plan on opening issues to address those work items as feature requests/bug reports. Should I put them all into one issue, or each item in its own issue?
2. I've forked the documentation repo to add Package Mode documentation. My plan is to first, create the issues as stated in question 1. Then, I will re-request review for this PR, and request review for my documentation PR in tandem. Does this workflow sound fine? (Note: the documentation PR will require the issues to be opened first, as I plan to link to them within a "limitations" section of the documentation for package mode).
3. For some reason, when coming back to this, the precommit hasn't been playing nicely with my git config regarding line endings. I've tweaked my git config to try to adjust, but that somehow ended up in CRLF line endings making their way into one of the bash scripts, causing it to fail. To remedy this, I just ran dos2unix on the whole repo - however, this has resulted in ~16 extra changed files on this PR. Each of those ~16 files seem to only have CRLF -> LF changes, and all the ones I've seen are only on specific lines rather than whole files. My question is - would you like me to try to revert my branch back a couple commits and deal with the line endings problem I faced in a more focused way than just running dos2unix, or is it fine to leave the branch as-is?

Thank you so much to the maintainers. Again, I apologize this has taken so long, and I really appreciate your patience and assistance throughout this whole process.

[webbertakken]

Just heading out but I can answer quickly now:

1. Prefer separate issues where possible
2. Sounds fine to me
3. If you're using auto-crlf you shouldn't have this problem. I'll list my config below. Ideally we wouldn't have 16 files change that aren't related, but if it really doesn't work then it's no disaster.

# `~/.gitconfig

# ...

[core]
eol = native
autocrlf = input

# ...