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

Revert "Test Godot with Vulkan in CI" #48024

Closed
wants to merge 1 commit into from

Conversation

madmiraal
Copy link
Contributor

This PR reverts #47414, because there are a number of issues with #47414 the most important being:

  1. It relies on separate test projects. While test projects are useful in identifying problems, flaws in those test projects can cause CI tests to fail, which will prevent PRs from being merged until the test projects are fixed. Note: This is different to the unit tests that are part of Godot, because a flawed unit tests can be updated as part of the same PR that is fixing an issue. Similarly, compat breaking changes can include updates to unit tests. This is not possible with separate test projects.
  2. It includes third-party test projects. This makes Godot at the mercy of those third-party projects. Changes to those projects are not subject to the same change controls as Godot Engine controlled projects. Uncontrolled changes to these projects can cause CI tests to fail, which will prevent PRs from being merged until changes to the test projects are fixed.
  3. It includes a personal build (not even a fork) of (SwiftShader) software used for testing.

Note: Currently #47789 is failing CI checks because of #47414, not because #47789 is flawed, but because #47414 is flawed.

While I fully support the principal behind the updates to the CI checks, I think the updates introduced in #47414 were not fully developed or thought through. Therefore this PR should be reverted until the issues identified above have been properly addressed.

@Zireael07
Copy link
Contributor

If we remove the test projects, what other ways of CI testing and catching regressions do you propose?

To fix the issue of "uncontrolled" & "third-party projects", I imagine the projects used to test could be transferred to godot-engine organization?

@qarmin
Copy link
Contributor

qarmin commented Apr 19, 2021

As I wrote in the previous thread, I agree with the statement that using an external project in Godot is not a very good idea, so I would prefer Godot to create an official test project instead removing it completelly.

Godot has unit tests, but their number and coverage are small because for many elements e.g. physics it is not possible to create such tests easily.

The current test project tests the editor (opens and closes it), all classes and methods (also those that PR changes and adds), adding and removing all nodes etc.
Until merging PR I've been testing some bigger PRs manually and I've found with its help some quite significant bugs that had the possibility to be included in the Godot main repository and stay there for a longer period of time e.g. #47727, #47569, #45852, #45672, #43725, #43323.
Thanks to CI such bugs are automatically found, not allowing to include not pre-tested code into repository.

As for the fact that a change in a test project can trigger errors in CI, that's true and even happened once(yeah, that was my fault).
However, since this time added my own, more restrictive CI to project(e.g. 1 hour long project testing).
If there are any problems with a project, removing it from CI is a matter of one simple commit.

Of course there is also the matter of PR's which break compatibility (at the moment I can think of only 3) and as I wrote earlier this can be handled with 1/2 find/sed commands in CI e.g. find test_project -name "*.gd" -exec sed -i "s/OS/Platform/g" '{}' .
It may not be very elegant, but it's simple, and for me it's a very small price to pay for early error detection.

As for SwiftShader, I don't see any problem here since it works as it should(in Ci there is info which commit it uses).

@Xrayez
Copy link
Contributor

Xrayez commented Apr 19, 2021

@qarmin's test projects seem quite useful even for non-official stuff, see resolved issues like goostengine/goost#61 in a custom module. I've been considering integrating something like this for the reasons above.

But yeah, more reproducibility would be needed, and the approach has to mature to the point of not interfering with checks in other pull requests.

An ideal solution to this would be implementing something like godotengine/godot-proposals#168, where issues could be reported outside of CI, or accompany the CI results...

That said, if there's a way to make current tests to work in non-blocking manner while still being able to report issues coming from those checks, this would likely resolve the underlying problem as presented with this PR. But at the same time, I don't think the downside of CI occasionally failing to be a major problem (as long as those blockers are fixed, of course).

@Xrayez
Copy link
Contributor

Xrayez commented Jul 12, 2021

Looks like current CI checks work fine, and this PR has merge conflicts now. Would you say it's still relevant?

@akien-mga
Copy link
Member

Indeed, IIRC qarmin made the CI project more resilient to such API changes so that it doesn't disrupt the workflow.

There's still some points worth discussing (notably moving the project under @godotengine) but a revert is not needed.

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.

6 participants