search

Unity-Technologies / EditorXR

Author XR in XR
Other
928 stars 166 forks source link

Fix tests for Cloudbuild #503

Closed mtschoen-unity closed 5 years ago

mtschoen-unity commented 5 years ago

This PR removes the BuildPlayerTest suite in lieu of using CloudBuild, and fixes the SaveProjectSettings_UpdatesProjectSettingsFile test which was failing on cloudbuild. The Ring.cs file from the mouse feature wasn't wrapped in #if UNITY_EDITOR, so I fixed that, too.

amirebrahimi commented 5 years ago

Are we losing any coverage by removing the BuildPlayerTest suite? I can't tell just from a cursory glance.

mtschoen-unity commented 5 years ago

Are we losing any coverage by removing the BuildPlayerTest suite? I can't tell just from a cursory glance.

We can set up build configurations to build whatever players we want on CloudBuild. You can't run tests without doing at least one player build, so if we want to test >1 platform, we just set up different configurations, and the added bonus is that we get separate download links (exe, .apk, etc.) for each one, if we want to locally test the resulting players.

I removed the tests because they were actually causing errors on CB. I can't remember the specifics but it's easy to reproduce by just pointing the build config at current development.

The only downside is that we can't run a "build all supported players" test locally, but my thinking is that if we have CI, we don't have to do that anymore.

amirebrahimi commented 5 years ago

Hmm... Well, in that case it'd be interesting to know what errors were being caused. It's still useful to do a quick test locally to see if any build issues happen, no? Especially for a quick sanity check. Otherwise, you'll have to round-trip with CB to see if it builds OR install the other platforms and build.

mtschoen-unity commented 5 years ago

Hmm... Well, in that case it'd be interesting to know what errors were being caused. It's still useful to do a quick test locally to see if any build issues happen, no? Especially for a quick sanity check. Otherwise, you'll have to round-trip with CB to see if it builds OR install the other platforms and build.

Yeah I'll double-check when I have a chance. I'm also wondering if there's some attribute or flag to ignore a test on CB and still allow it to exist in the repo. There is one to always ignore a test which we could include and just have you comment out for a local test.

Bear in mind that to test the local build, you would definitely need the platform installed, and you can still do a build whenever you want--you just don't have the utility to run more than one platform at once.

mtschoen-unity commented 5 years ago

Actually, I added them back and they pass just fine. I guess it's a non-issue! 👍

mtschoen-unity commented 5 years ago

@amirebrahimi one last thing before I merge this: Apparently there is actually a UNITY_CLOUD_BUILD define that we can use to just cut out the entire BuildPlayerTest file on CloudBuild. This allows us to include the test locally, but not waste our infrastructure on redundant tests :)