-
Notifications
You must be signed in to change notification settings - Fork 0
OGSMOD-7812 - Centralize the asset access for unit tests #114
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
| { | ||
| static const std::filesystem::path filePath = TestHelpers::getOutputDataFolder(); | ||
|
|
||
| const std::filesystem::path filePath = TestHelpers::getOutputDataFolder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No static in case there is a need to change it for a specific unit test.
| int width() const { return _width; } | ||
| int height() const { return _height; } | ||
| bool presentationEnabled() const { return _usePresentationTask; } | ||
| const std::filesystem::path& dataPath() const { return _backend->dataPath(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used.
|
erikaharrison-adsk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments/questions. If it passes all the tests, etc. seems like fine changes, thanks.
| glfwWindowHint(GLFW_DOUBLEBUFFER, GLFW_TRUE); | ||
| glfwWindowHint(GLFW_CLIENT_API, GLFW_OPENGL_API); | ||
|
|
||
| #ifdef GLFW_SCALE_FRAMEBUFFER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the last commit merged. In another context the code uses an older glfw not having this setting.
| throw std::runtime_error("Failed to initialize the unit test backend!"); | ||
| } | ||
|
|
||
| _backend->setDataPath(localAppPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously this was a scoping-like action (should've been RAII). If we no longer need it, great, but sanity check that its all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, validated locally on macOS & Windows.
|
|
||
| void AndroidTestContext::init() | ||
| { | ||
| std::string localAppPath = getenv("HVT_TEST_ASSETS"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously android impl had been using symlinks - not sure if this is what that was. If things work now, great! Thanks for tidying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestHelpers::getAssetsDataFolder() is doing that.
Description
The pull request refactors a little bit the code to use available helpers.
Tests Performed
Checklist