aws-samples / aws-ambit-scenario-designer-ue4

AWS Ambit Scenario Designer for Unreal Engine 4 (Ambit) is a suite of tools to streamline content creation at scale for autonomous vehicle and robotics simulation applications.
https://aws-samples.github.io/aws-ambit-scenario-designer-ue4/
Apache License 2.0
85 stars 21 forks source link

Separate integration tests from unit tests #17

Closed JuliaABurch closed 2 years ago

JuliaABurch commented 2 years ago

What was the problem/requirement? (What/Why)

Currently, the build pipeline isn't set up to be properly licensed to successfully run the Houdini tests. In the meantime, we'd like some way of running the unit test suite from the command line without these.

What was the solution? (How)

The naming structure of the tests have changed to more explicitly move all tests under "Ambit.Unit" while the Houdini tests live under "Ambit.Integration". The "AmbitUtils" tests have moved to "Ambit.Unit.Utils".

When running the tests from the command line, Unreal only does forward string matching - passing in "Ambit" would run all of the tests (both organized under Unit and Integration), while "Ambit.Unit" would run all of the unit tests without running Ambit.Integration.

I didn't want to explicitly list all the tests that should be run, as developers adding new unit tests would have to remember to add them to the pipeline. There is no concept of a negative filter - I can't tell it 'run all tests that match this string, but not these specific tests."

Similarly, the only filter that can be used from the command line is represented by the flags that fall under the FilterMask -- Smoke, Engine, Product, Perf, Stress, and Negative. All Ambit tests are marked as Product tests by default, which is IMO the right place for them - there's no separate category that really suits the integration tests.

What artifacts are related to this change?

Issues: Ambit-10

What is the impact of this change?

Developers running tests will notice that they are organized under a new structure, and will have to expand one more node to view class-specific tests. Otherwise it is just as simple to run all of the Ambit tests.

Are you adding any new dependencies to the system?

None

How were these changes tested?

How does this commit make you feel? (Optional, but encouraged)

Fairly ambivalent


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

rukekre commented 2 years ago

Is there not a way to specify multiple tests at a time? For example, "Ambit.Unit" and "AmbitUtils.Unit" simultaneously. Otherwise, we may want to formalize how we define tests: "Ambit.{TestType}.{DirectoryName}.{TestSuite}.{MinorTestSuite}" might be better, as our current schema defined here confuses AmbitUtils a bit. This would make the AmbitUtils "Ambit.Unit.AmbitUtils.MathHelpers" and other tests (AmbitWorldHelpers for example) "Ambit.Unit.Ambit.AmbitWorldHelpers" This is a minor concern, ultimately, but worth double-checking before we commit.

@brhook-aws The way I understood this was that running with Ambit.Unit will trigger all Ambit.Unit and Ambit.Unit.Utils tests, since AmbitUtils tests have moved to Ambit.Unit.Utils

@JuliaABurch can correct me if I'm wrong

JuliaABurch commented 2 years ago

Unfortunately, Unreal doesn't let you pass in multiple "test paths" from the command line - if you pass it a list, it will only run the first one from the list.

I agree that we should be consistent with test organization and naming, and it's definitely worth discussing! For reference, the change I'm proposing is essentially "Ambit.{TestType}.{TestSuite}" - it's also worth noting that changing test organization/names is fairly easy to do, as this change shows - it's merely changing a string in the test file and only requires a rebuild to reflect changes.

I'm not sure we want to include {DirectoryName} as imo it doesn't make the test more readable (what does knowing the directory name tell me about the test?), and it's something that will be easily missed with refactoring if/when files get moved into different directories.

Similarly, I don't know that repeating "Ambit" (in "Ambit.Unit.Ambit.{TestSuite}" or "Ambit.Unit.AmbitUtils") improves readability. I see that this preserves the original structure - originally, Ambit tests were distinct from AmbitUtils tests - but as they're all packaged in the same "Ambit" plugin, and I don't see customers using "AmbitUtils" separate from the Ambit plugin, I actually didn't see the benefit of completely separating them to begin with.