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

Moved ability and move effect tests to folders by letter #5234

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AsparagusEduardo
Copy link
Collaborator

Description

I started realizing that after adding TODO tests, the amount of test files grew a lot, so I made this PR to help organization.

Feature(s) this PR does NOT handle:

Only moved the files, didn't do anything else to make this easier to review.

Discord contact info

AsparagusEduardo

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 20, 2024

is there a reason why the size of the folder would bother us?

Because I generally think this is a bad change for ppl who write tests.

@AsparagusEduardo
Copy link
Collaborator Author

is there a reason for this change?

Easier to browse on interfaces such as VSCode. Whenever I open the move_effect or ability folders, they always start on top, making it harder to search for a specific file. With 352 effects and 311 abilities, scrolling quickly becomes a pain.

Before:
image
After
image

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 20, 2024

why not search with CTRL+P?

@AsparagusEduardo
Copy link
Collaborator Author

is there a reason why the size of the folder would bother us?

Because I generally think this is a bad change for ppl who write tests.

Bad how? We already have folders to split tests and I don't recall any complaints with them

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 20, 2024

is there a reason why the size of the folder would bother us?
Because I generally think this is a bad change for ppl who write tests.

Bad how? We already have folders to split tests and I don't recall any complaints with them

I just think it is redundant to introduce an other folder. It just makes it obscure.

@AsparagusEduardo
Copy link
Collaborator Author

I just think it is redundant to introduce an other folder. It just makes it obscure.

If you use Ctrl+P, you'll find the file anyway. If you don't, you can find the file by its name in the same way

@AlexOn1ine
Copy link
Collaborator

Mentioned on discord but It would be better if someone else reviews the pr.

@hedara90
Copy link
Collaborator

Just adding my 2Ꝑ to this.
It doesn't really matter for me how the test folders are laid out, because quite often I have to find what I'm looking for by running grep -nri "MOVE_WHATEVER" test/ and then opening the file I find it in. Because it's not guaranteed that what I'm looking for will be where I'd expect it to be.
If I for example want to test Mud-Slap, should I look in battle/move_effect_secondary/ since it's a secondary effect? Or should I look in battle/move_effect/ since it's using MOVE_EFFECT_ACC_MINUS_1 since it's tested with Sand Attack?
So for me it doesn't really help to sort it into folders, it'll be an additional letter and / I have to type to open the file for editing.

@pkmnsnfrn pkmnsnfrn marked this pull request as draft August 28, 2024 03:01
@AsparagusEduardo AsparagusEduardo added type: cleanup category: battle-tests Related to the automated test environment labels Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: battle-tests Related to the automated test environment type: cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants