-
Notifications
You must be signed in to change notification settings - Fork 77
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
[USDU-249] Adds test case for InitUSD #330
base: dev
Are you sure you want to change the base?
Conversation
0e16236
to
33342e6
Compare
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.
I think we need to rethink how to thoroughly test the InitUSD.Initialize test without it impacting/ being impacted by the other tests having the plugin initialised. Perhaps we could make a fake instance (not sure how it would interact with the real instance though) or maybe we need a test outside of UTR...
} | ||
|
||
[Test] | ||
public void InitUsd_Initialize() |
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.
Please add the expected result to the end of the test name, something like "InitUsd_Initialize_Succeeds"
// Reset 'm_usdInitialized' for accurate testing | ||
ResetInitUsd(); | ||
|
||
Assert.True(InitUsd.Initialize(), "USD Initialize failed"); |
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.
Is there a more robust thing we can also check for, as well as the return value of Initialise()?
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.
eg check that the plugin is registered, that the expected debug log is printed, that no other errors are handled and logged.
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.
i've looked into this a bit more - on how to tell if the plugin has been registered and the typebinding
But I just ended up having more questions, do you know who I should be asking about the init steps?
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.
You can check that unity types have been added the TypeBinder, that usd error message are logged in Unity console and that the plugins Registry is not empty. We can chat if you have more questions.
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.
Thank you @judubu - I will contact you via slack!
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.
After talking with Julien, it turns out the PlugRegistry is not behaving correctly - created a separate JIRA ticket for this
https://jira.unity3d.com/browse/USDU-295
Will continue work on this PR after the above JIRA ticket has been solved
public void InitUsd_Initialize() | ||
{ | ||
// Reset 'm_usdInitialized' for accurate testing | ||
ResetInitUsd(); |
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.
I'm not sure that just changing the bool is enough here. Initialize() does stuff that might affect this test, such as setting up the UnityTypeBindings for USD. I'm not sure what the answer is though :D
Purpose of this PR
USDU-249
Testing
Adds test cases for InitUsd script
tries to check if an early throw / exit is done when given wrong parameter value for function InitUsd.SetupUsdPath()
Complexity:
1
Halo Effect:
1