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

[CI] Run unit tests on desktop release templates #93780

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jun 30, 2024

The resulting artifacts are a bit larger, but I'd argue it's worth the cost to help identifying various issues in release templates and tests, I'd really prefer having a debug template build as well to run unit tests on but I think that's a bit excessive

The fixes for the minimal UI build are largely band-aids, I've not attempted to fix or really test the viability of the disable_advanced_gui mode beyond making it run correctly in tests, though I might try work some on that later, but it's outside of what my general use cases personally are

The second commit shouldn't be controversial but kept it separate just in case, both of them entirely depend on PopupMenu so it doesn't really make sense to have them exposed in this case

I haven't tested other combinations of build options to identify possible edge cases, but this makes the minimal release (and debug) templates test fully

Can reduce the scope and just run/compile tests on the Linux builds, if desired, haven't run into any platform specific issues in the tests when running this, but I think it's worth the few megabytes to catch any weird platform specific issues that might crop up

@@ -90,6 +92,7 @@ TEST_CASE("[HTTPClient] verify_headers") {
ERR_PRINT_ON;
}

#if defined(MODULE_MBEDTLS_ENABLED) || defined(WEB_ENABLED)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't expect tests to be run on web (I don't even know how to do that) but the dedicated test is because web has its own http client, so just made this change for completeness

#include "tests/servers/test_navigation_server_2d.h"
#include "tests/servers/test_navigation_server_3d.h"
#endif // MODULE_NAVIGATION_ENABLED
Copy link
Member Author

Choose a reason for hiding this comment

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

Some of the nav tests could be run without the module theoretically to confirm default values, and that could be a future improvement to put some of the test code in those files in their own #ifdef MODULE_NAVIGATION_ENABLED blocks to split it off, but kept it simple here, I think we can trust that the basic default stuff of the navigation "works" when navigation is disabled

@AThousandShips AThousandShips removed the request for review from a team June 30, 2024 13:18
@akien-mga
Copy link
Member

The second commit shouldn't be controversial but kept it separate just in case, both of them entirely depend on PopupMenu so it doesn't really make sense to have them exposed in this case

I would suggest moving it to its own PR, so the second commit doesn't end up having to undo the workarounds adding in the first.

@AThousandShips
Copy link
Member Author

Will do!

@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 1, 2024

Discovered a few tests have been missed and aren't run, will make a PR adding them on top of this

Edit: seems to just be one so will add it to this

Edit 2: Fixed

@AThousandShips AThousandShips marked this pull request as draft July 1, 2024 08:50
@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 1, 2024

The output on the windows release isn't printing correctly so will try and fix that and see what might be wrong

Edit: Might be fixed by using the .console exe, seeing if it runs correctly now

Edit 2: Works now, prints correctly, only added for the release version as the editor is built with windows_subsystem=console

@AThousandShips AThousandShips force-pushed the unit_test_release branch 2 times, most recently from 4744372 to fbde1c2 Compare July 1, 2024 09:10
@AThousandShips AThousandShips marked this pull request as ready for review July 1, 2024 09:22
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Nice work! Even though this ideally should be split in 2 PRs, I think it's fine like that.

@AThousandShips
Copy link
Member Author

I think having it as a single PR reduces the risk of new issues arising between the base fixes and the enabling of the CI making that PR still include fixes, and makes it easier to verify the tests

There are already a few PRs open that will fail with this being merged, so they'd have to be fixed by the second PR if they happen to be merged at the wrong time

But can split if desired, but I think it makes more sense with one PR

@akien-mga
Copy link
Member

But can split if desired, but I think it makes more sense with one PR

I'd suggest to at least split in two commits which can stay in this PR. First a commit to do fixes in engine code, then enabling new CI tests.

@AThousandShips
Copy link
Member Author

Will do!

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 18, 2024
@akien-mga akien-mga merged commit 4d238b3 into godotengine:master Jul 18, 2024
17 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the unit_test_release branch July 18, 2024 13:47
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants