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

GD-607: Fix discovery of root test suites #604

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

esainane
Copy link
Contributor

@esainane esainane commented Nov 28, 2024

Why

The GdUnit4 Godot Editor plugin currently does not find any tests in the project root directory, even when configured to do so.

Fixes #607.

What

  • Refactors external calls to scan_test_directories into a new scan_all_test_directories instead. These external calls all had the form scan_test_directorys("res://", GdUnitSettings.test_root_folder(), []). This provides a cleaner API, and a logical place to add handling for the "include all resources" special case.
  • When scan_all_test_directories is called to determine which resource paths should be included, and the project is configured to include all resources, it early returns ["res://"]. Otherwise, it performs the current subfolder scanning logic via scan_test_directories("res://", GdUnitSettings.test_root_folder(), []).
  • Fixes a typo: suits -> suites
  • Fixes a typo: scan_test_directorys -> scan_test_directories

@MikeSchulze
Copy link
Owner

Hi,

thanks for contributing to the project. 👍

Please, before fixing a bug you have to create a bug issue and describe the problem, then build your branch and commit your pull request.
The PR should only contain the Why - small problem description and a What - describe what is changed in short points
And finally link the bug issue
image

Please create a bug report and move the details described here to the bug report.

Thanks for your work, I'll take a look at it later

Copy link
Owner

@MikeSchulze MikeSchulze left a comment

Choose a reason for hiding this comment

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

Looks good, please add test coverage for the new code path you implemented.

@MikeSchulze MikeSchulze added the bug Something isn't working label Nov 30, 2024
@esainane
Copy link
Contributor Author

esainane commented Dec 2, 2024

Thank you for the guidance! I'll see about structuring this properly soon.

@MikeSchulze MikeSchulze changed the title Fix discovery of root test suites GD-607: Fix discovery of root test suites Dec 3, 2024
@MikeSchulze
Copy link
Owner

Thank you for creating and assigning the error problem. To finally fix the bug, please add the missing test case.

Thank you again for your work.

Copy link
Owner

@MikeSchulze MikeSchulze left a comment

Choose a reason for hiding this comment

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

Sorry, but we should avoid default arguments here.

addons/gdUnit4/src/core/command/GdUnitCommandHandler.gd Outdated Show resolved Hide resolved
addons/gdUnit4/src/core/command/GdUnitCommandHandler.gd Outdated Show resolved Hide resolved
@MikeSchulze
Copy link
Owner

@esainane do you continue here?

@esainane
Copy link
Contributor Author

esainane commented Dec 7, 2024

I do. My apologies for the delay, I should be free to finish this up shortly.

esainane and others added 8 commits December 10, 2024 05:27
The gdUnit4 settings description for `Test Lookup Folder` states:
> Subfolder where test suites are located (or empty to use source folder directly)

However, leaving this setting empty (or set to /, or res://) will cause
gdUnit4 to not find any test suites in the root folder.
`scan_test_directories` starts by examining subfolders, though it is
very permissive with any of these test directory settings.

This refactors external calls to `scan_test_directories` into a
`scan_all_test_directories` instead, which performs the common call if
any specific test directory is set, and returns the base directory
otherwise.

With this change, it will now correctly pick up a `[TestSuite]`
annotated class at `res://Test.cs`.

This would still be bad practice in a proper project, since it has to
perform a lot of scanning which a dedicated test directory would avoid.
However, this should eliminate much confusion when trying to create a
minimal reproducible test project and wondering why the test suite
is silently not being detected.
`scan_all_test_directories` will still retrieve the value from settings
if not otherwise specified.
@esainane esainane force-pushed the root-cause-analysis branch from 9189572 to 959b9b1 Compare December 10, 2024 05:32
Copy link
Owner

@MikeSchulze MikeSchulze left a comment

Choose a reason for hiding this comment

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

Good job!

@MikeSchulze MikeSchulze merged commit 2ec28a1 into MikeSchulze:master Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GD-607: GdUnit4 Godot plugin does not find test suites at the root folder
2 participants