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 yaml support for tests and glTF tests #387

Merged
merged 7 commits into from
Jul 11, 2023
Merged

add yaml support for tests and glTF tests #387

merged 7 commits into from
Jul 11, 2023

Conversation

mattelser
Copy link
Contributor

@mattelser mattelser commented Jul 7, 2023

Fixes #308 . This sets up a workflow that I expect to use for future tests: establish expected results via a yaml config, then compare some function call to that expectation, repeat in various scenarios.
In this case, the "scenarios" are all different gltf models from the Khronos Group glTF asset generator meant for exactly such testing.

The best place to start a review will probably be in ExampleTests.cpp, as this demonstrates (and simultaneously documents) the new workflow.

}

ConfigMap getScenarioConfig(const std::string& scenario, YAML::Node configRoot) {
ConfigMap sConfig = ConfigMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Could you come up with a better name for this variable than sConfig? Perhaps scenarioConfig?

Copy link
Contributor Author

@mattelser mattelser Jul 11, 2023

Choose a reason for hiding this comment

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

At the risk of bikeshedding: it would appear we have slightly different preferences for variable name length. I generally agree with your "it's not 1990" philosophy of properly spelling things out, which you mentioned the last time this came up. I usually aim for a "clear names, smaller where harmless" approach (partly because shorter lines means I can cram more tabs next to each other without scrolling/wrapping 😄 ). I think having the one config in a small function called getScenarioConfig be named sConfig is clear and concise, but am happy to conform to whatever the preferred style is!

// YAML Config Examples
// ----------------------------------------------

std::string transmogrifier(const std::string& s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the name.

@lilleyse lilleyse mentioned this pull request Jul 7, 2023
@lilleyse
Copy link
Contributor

@mattelser could you add "Fixes #308" to the PR description so that it gets linked to that issue?

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

This looks pretty good overall. One the style side, try to use auto, const, and references wherever applicable. I pointed out the areas I noticed but there could be others.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
ThirdParty.extra.json Outdated Show resolved Hide resolved
tests/ExampleTests.cpp Outdated Show resolved Hide resolved
tests/testUtils.cpp Outdated Show resolved Hide resolved
ThirdParty.extra.json Outdated Show resolved Hide resolved
tests/GltfTests.cpp Outdated Show resolved Hide resolved
tests/GltfTests.cpp Outdated Show resolved Hide resolved
tests/GltfTests.cpp Outdated Show resolved Hide resolved
@lilleyse lilleyse merged commit 67c8b2a into main Jul 11, 2023
3 checks passed
@lilleyse lilleyse deleted the addGltfTests branch July 11, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create unit tests for GltfUtil.cpp
3 participants