Skip to content
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 Feature to test Unity Packages #164

Merged
merged 115 commits into from
Jul 3, 2023
Merged

Conversation

trudeaua21
Copy link
Contributor

@trudeaua21 trudeaua21 commented Feb 23, 2022

Changes

  • closes Feature: Tooling to run tests for Unity packages #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 have jq installed
    • 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

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

Pulls in the changes from the main repo
@trudeaua21
Copy link
Contributor Author

trudeaua21 commented Jun 9, 2023

@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
Copy link
Member

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

# ...

@trudeaua21
Copy link
Contributor Author

Alright - I've finished up all the documenting that I can think of, and filled out a PR for documentation of this feature in the documentation repo.

I'll need some input on that PR from the maintainers - I left it in draft status since I couldn't get the documentation repo to build locally for some reason, but I've made all the changes I intended to, so it's only in draft status out of formality given the contribution guidelines.

But great news - this PR is ready for review again! 🚀

@davidmfinol Tagging you for visibility since I wasn't able to re-request your review - sorry if that wasn't necessary!

@@ -0,0 +1,11 @@
{
"name": "fake.notarealpackage.RuntimeTests",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer example.com or com.example instead of using actual website names.

That domain is meant for such uses specifically.

Copy link
Contributor Author

@trudeaua21 trudeaua21 Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good call! I've addressed this in my most recent commit - the package is now called com.example.testpackage.

It has ~40 changes, but that's mainly just because I renamed the folder for the package to the new package name. The key changes are:

  • Renamed the package folder itself
  • Updated the package.json with the new package name & updated the info in it to better match the new name
  • Updated all the assembly definitions to match the new name
  • Updated the main.yml tests to point at the new package name

If that was the only note, then I have nothing else to add 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Looks good to me.

Probably good if some of @davidmfinol, @AndrewKahr, @frostebite, @GabLeRoux, @lazerwalker could have a look. And then hopefully we can release it soon :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a journey! I just caught up with the history on this one. I don't see any major issues with the limitations, jq is a pretty common requirement, using only Linux runners for the time-being seems fine for a initial release of a new feature.

Seems like there have already been several users who have used the fork successfully with all issues reported already having been addressed.

Great work!

Copy link
Member

@AndrewKahr AndrewKahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

Really really amazing work, I know this feature has been talked about a lot and so finally getting support for it will be a huge step forward!

Again awesome work!

@webbertakken webbertakken merged commit 7787abf into game-ci:main Jul 3, 2023
@webbertakken
Copy link
Member

Released in https://github.com/game-ci/unity-test-runner/releases/tag/v2.2.0

@timcassell
Copy link
Contributor

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.

This was a year ago, but Unity's latest stunt almost makes it seem like you saw the future!

Kuoste added a commit to Kuoste/fi.kuoste.terraintile that referenced this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Tooling to run tests for Unity packages