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

Add tests for get_node autocompletion #86973

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

HolonProduction
Copy link
Member

Requires #86961

@Mickeon
Copy link
Contributor

Mickeon commented Jan 8, 2024

A whooping 100 files added. Are they all truly necessary for the tests?

@HolonProduction
Copy link
Member Author

A whooping 100 files added. Are they all truly necessary for the tests?

Yeah the file count kinda exploded 😅. But every idea I have to reduce it seems worse.

  • Each test needs at least 2 Files. That makes 50 test, but some of them are duplicated in different call situations (yes this is needed, see for example #72328 and there also was another one that i cant find rn 😞). So we are at ~30 test, and I plan to do more, not reduce them. For something as complex as this, reducing tests doesn't seem like a good idea.
  • I often have test which require the same output for multiple files. But specifying script paths inside of the cfg files, would make it harder to find the right cfg. And I don't wan't to have multiple scripts inside of the same file, since it would introduce yet another "magic" character to separate them.
  • We could reduce each test into one file, but this would either require a sort of preface for the GDScript and parsing that would be annoying. Or we could embed the GDScript inside the cfg, but that would make it impossible to test something manually by opening the script in the editor.

If you have any other ideas to reduce it, I'm all in for it, that being said many test files aren't that big of a problem to justify any of the tradeoffs that I listed IMO.

@HolonProduction HolonProduction force-pushed the tests-batch-1 branch 4 times, most recently from f0ab094 to 483f0cb Compare January 21, 2024 22:04
@HolonProduction HolonProduction force-pushed the tests-batch-1 branch 20 times, most recently from 0cd365d to a1562d3 Compare January 27, 2024 01:23
@HolonProduction
Copy link
Member Author

Waiting for #87759 in order to fix the CI.

@HolonProduction HolonProduction force-pushed the tests-batch-1 branch 2 times, most recently from e45abea to 875a601 Compare March 1, 2024 11:59
@HolonProduction HolonProduction marked this pull request as ready for review March 1, 2024 12:34
@HolonProduction HolonProduction requested a review from a team as a code owner March 1, 2024 12:34
@HolonProduction HolonProduction changed the title [WIP] Add tests for get_node autocompletion Add tests for get_node autocompletion Mar 1, 2024
@HolonProduction
Copy link
Member Author

Ready for review

@Mickeon Mickeon requested a review from AThousandShips March 1, 2024 12:52
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

On a cursory look it is good to me. Thanks for working on this!

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 4, 2024
@akien-mga akien-mga merged commit 7be96a5 into godotengine:master Mar 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@HolonProduction
Copy link
Member Author

Oh shit, I just noticed I forgot to squash the commits and now you merged it 😅. I guess it can't be changed now?

@akien-mga
Copy link
Member

Yeah I missed that. It can't be changed once merged.

@HolonProduction HolonProduction deleted the tests-batch-1 branch October 5, 2024 18:58
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