Test case improvements and updates #2393
Conversation
Half2 now has proper ToSting (before it would simply output the type instead of the values with ToString). Converter seems to similarly match formats for other converts to a degree
- Need verification on how the converters should be changed (if they even still need to be) - All half tests now pass
Test now passes but a bit confused as to what this test is trying to do. Will need context with PR. Didnt have empty constructor therefore it was impossible for this to ever pass.
Relates to a possible rework in GameTestBase and SendImage method
10 Windows Tests Updated 11 Windows Tests Updated RunTestGame AMD test animation images are now uodated RunTestGame seems to be using older images for animation tests SpriteRender2DTests has outdated images Updated SpriteRenderer2DTests.f1 and Tests files (Test was failing for small pixel differences) SpriteRenderer3DTests Updated Images - Images were pixels off causing a failure SpriteRotationTests updated images - Images were pixels off causing a failure SpriteTestGame updated images - Images were pixels off causing a failure TesselationTests updated images - Images were pixels off causing a failure SceneAmbientLight test has outdated images Updated SceneAmbientLight image
…t succeed Unsure why this originally was set to false after reading documentation, might be missing some context. Can simply add a TODO above line 37 instead to give more context. The rest of the test seems to function as expected.
- Outdated images for various tests inside Particles.Tests.Windows Stride.Graphics.Tests - Updated image for TestRenderToTexture Test - Outdated images for AMD
…ditional info for why they fail info
For whatever reason, it was case sensitive and could not find the corresponding template packages. We not hit another error for these tests but it gets much farther now.
This reverts commit 54e0570.
This reverts commit dc984b5.
For the time being, since these tests are designed for the teamcity agent and that isnt operational, these can be skipped to avoid large amount of failures. Note: May need feedback on these, might be okay to not gave these skip, regardless of if they always fail.
While this is true, you can check if the graphics tests pass locally using your own GPU. When you run the tests, it will create in the If you copy that directory besides the one named Tell me if you have any question and I'll try to reproduce it and explain it better. |
| Assert.False(CanPin<AutoLayout>()); | ||
| Assert.True(CanPin<AutoLayout>()); |
There was a problem hiding this comment.
Let's ask the author about this, hopefully he has time to get back to us regarding this line.
@ericwj do you remember why you introduced this specific assertion ? As far as I can tell empty autolayouts have always been pinnable ?
There was a problem hiding this comment.
Oh you pinged me a while ago. I must've completely missed the mention. Sorry about that @Eideren.
I'm pretty sure if it was part of the PR and got accepted, the test passed. And indeed it does on .NET Framework, which is the easiest for me to test without installing old .NET runtimes. It is very possible that it also passes on older .NET Core or .NET 5/6. But if it now doesn't pass and you adjusted it to that circumstance, it'll likely be fine.
It is testing behavior of the runtime and that appears to have changed sometime. It was convenient that this test returned false just to know that in some places nobody forgot to change struct layout from the default of auto, at least where pinning was attempted - I believe this could be the case in the animation infrastructure. I don't think there is any other impact of this change in behavior.
There was a problem hiding this comment.
Coincidentally I come accross another situation where a test crashes and because of this here, I checked and it happens that it is 2024 when I learn that already on .NET 2.0 an unannotated struct is in fact layout sequential. I must've seen this so many times in ILSpy or wherever... Not knowledge active enough to not surprise me. Somehow I assumed it to be auto, probably when writing the above code. At least when I wrote the reply...
sources/editor/Stride.Assets.Presentation/StrideDefaultAssetsPlugin.cs
Outdated
Show resolved
Hide resolved
Oh! Gotcha okay, so I guess with that kinda confirmed, I think my confusion about these were more so seeing only the GTX 960 directories listed in the project. So essentially the workflow would be to first test and save out your current GPU directory from the master branch and then copy those out into your branch you have your changes in? I think what confused me was no one else pushes their GPU directories. So ideally I should roll those changes back and remove the skips. And I shouldnt push the copied directory with the GPU tests also right? I guess if everyone did that we would have a ton of test directories. Thank you for the insight! Also followup cause I am newer, should this info of this workflow of rendering tests be written down anywhere? And would this workflow change when the teamcity agent is back up? |
The way I do it usually is the following:
I however, would only do this locally when modifying the graphics or rendering parts of the engine, which would be the time where these things are useful to verify no regressions have happened. Ideally, a complete set of reference images more current than those for the Also, I believe that the reference images for the
The
I also don't think it would be a good idea to push other directories for other GPUs to GitHub because that would be many many different directories, each one full of big but very similar images, and it would only benefit the people with exactly those exact GPU configurations.
Exactly.
Yes. I would document it. Haven't done it yet because it is a not very useful process. Currently, it only serves to verify you don't break things locally using your own GPU. |
Awesome! Ok cool, yes that cleared up everything. Thanks for the fast reply! Would it be a good idea to remove those extra images you had mentioned? I did notice that as I was looking at this.
What would be your recommendation for how to approach documenting this? Would simply adding comments suffice or would documentation somewhere need to be updated? Once again, thanks for the insight and help! |
I'd say yes. But better to ask someone that knows better where those images came from and if it's a good idea to delete them. Maybe @xen2 or @Kryptos-FR
As having a good test suite is good for the project, I'd say we should document it properly. If this changes in the future, we'll see, but for now we should aim to shed light on everyone of the dark corners of the Stride project (there are a few). Right now I think the best you can ask for all documentation things is @VaclavElias. He has done a great job with the official docs.
Always a pleasure to help! |
|
Thanks ! |
PR Details
Goal was to try and go through failing or skipped test cases and provide some general maintenence and fixing tests that constaintly fail. I feel like for some of these, there may be some discussion about if they should be skipped or not if they always fail.
Overall in this PR, I got 7 Tests to now pass that could not pass before (or where skipped).
4 in TypeConverter Half tests
LifetimeNoSimpleConstructor
TestGCHandleAlloc
TestGetFontInfo
Added 41 Skips for tests that will always seem to fail. These are all rendering related. Due to the fact that GPU rendering is not deterministic, devs would need a NVIDIA GeForce GTX 960 on their own personal workspace for these tests to pass. However these will be used in the future when the teamcity agent is back up and running and these tests will be no longer skipped. The images will also most likely outdated. Reviews of this PR might indicate that these skips should be removed and instead a note for others that they should fail if the proper hardward isnt used.
Note: There was a push that was related to updating the rendering test images, that was reverted and is not in this PR but reviewers may see references to a push that involved updating images.
1 Improvement to a test suite:
Fix related to change in StrideDefaultAssetPlugin
I was able to fix an issue that was occuring all the tests in the Stride.Samples.Tests suite. Originally I was seeing that the tests could not find certain directories which turned out to be case sensitive. After this change, the tests can now get farther but there is now a new error they hit which appears to be related to dotnet package issues. I think the scope of this test case failing exceeded what I was originally set out to do so for now I'll include this change for now. However I may need confirmation since I'm not sure if this current test suite failure is a result of my current workspace being set up incorrectly or not.
Related Issue
N/A
Types of changes
Checklist
(technically didnt add any NEW tests)
(It seems that the only area where I get super clear and consistent failures is Stride.Samples.Tests - about 32 test failures remaining)