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

Test regex names capitalization #206

Merged
merged 7 commits into from
Oct 11, 2021
Merged

Test regex names capitalization #206

merged 7 commits into from
Oct 11, 2021

Conversation

PabloLec
Copy link
Contributor

Prerequisites

Why do we need this pull request?

Regex names capitalization are now tested.
Some words are ignored:

  • If they contain already one upper case char (e.g: SSH, URL, TOTP) or I also think about brand names like iPhone.
  • If they contain one numeric char like ed25519 or say, i18n.
  • If they are specified in the test (for now "a", "of", "etc).

What GitHub issues does this fix?

Tell me what you think about the test and if you might have any false positive in my mind


for entry in database:
entry_name = entry["Name"]
for word in entry_name.split(" "):
Copy link

@ghost ghost Oct 11, 2021

Choose a reason for hiding this comment

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

Why not just using split()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, got used to make it explicit due to a previous project :p Fixed it

Copy link
Owner

@bee-san bee-san left a comment

Choose a reason for hiding this comment

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

Thank you so much!!! :)

Comment on lines 26 to 31
def test_name_capitalization():
database = load_regexes()

for entry in database:
entry_name = entry["Name"]
for word in entry_name.split(" "):
Copy link
Owner

Choose a reason for hiding this comment

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

Can we make a new test file test_regex_formating and move this test in there?

Also, can we move all other tests which test the format of that file? Like the rarity sorting 😄

if cleaned_word in ["a", "of", "etc"]:
continue

assert word.title() == word
Copy link
Owner

Choose a reason for hiding this comment

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

When this test fails, can we please explain why it failed? There's an example of this with the ^ $ regex test which tells you exactly why it failed :)

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.27%. Comparing base (669682c) to head (55b3465).
Report is 119 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
+ Coverage   92.21%   92.27%   +0.06%     
==========================================
  Files          13       14       +1     
  Lines        1631     1644      +13     
==========================================
+ Hits         1504     1517      +13     
  Misses        127      127              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Please move test_if_all_tests_exist() to test_regex_formatting.py

@PabloLec
Copy link
Contributor Author

Please move test_if_all_tests_exist()and test_sorted_by_rarity()to test_regex_formating.py

I just moved test_sorted_by_rarity(), I'm not sure if test_if_all_tests_exist() has to do with regex file formatting though ?

@bee-san
Copy link
Owner

bee-san commented Oct 11, 2021

Please move test_if_all_tests_exist()and test_sorted_by_rarity()to test_regex_formating.py

I just moved test_sorted_by_rarity(), I'm not sure if test_if_all_tests_exist() has to do with regex file formatting though ?

Make a new test file for testing the tests file? 👀

@ghost
Copy link

ghost commented Oct 11, 2021

Please move test_if_all_tests_exist()and test_sorted_by_rarity()to test_regex_formating.py

I just moved test_sorted_by_rarity(), I'm not sure if test_if_all_tests_exist() has to do with regex file formatting though ?

Make a new test file for testing the tests file? 👀

That would be too much files, maybe rename test_regex_formatting.py?

@bee-san
Copy link
Owner

bee-san commented Oct 11, 2021

Please move test_if_all_tests_exist()and test_sorted_by_rarity()to test_regex_formating.py

I just moved test_sorted_by_rarity(), I'm not sure if test_if_all_tests_exist() has to do with regex file formatting though ?

Make a new test file for testing the tests file? 👀

That would be too much files, maybe rename test_regex_formatting.py?

That's only 5 test files? If we want, we could put them into folders, no?

@PabloLec
Copy link
Contributor Author

PabloLec commented Oct 11, 2021

By the way, I guess we should also move test_check_keys_in_json from test_identifier.py to test_regex_formatting.py ?

@PabloLec
Copy link
Contributor Author

PabloLec commented Oct 11, 2021

Please move test_if_all_tests_exist()and test_sorted_by_rarity()to test_regex_formating.py

I just moved test_sorted_by_rarity(), I'm not sure if test_if_all_tests_exist() has to do with regex file formatting though ?

Make a new test file for testing the tests file? eyes

That would be too much files, maybe rename test_regex_formatting.py?

That's only 5 test files? If we want, we could put them into folders, no?

I think test_if_all_tests_exist() could stay in test_regex_identifier.py. I mean it's not off topic, it is testing if all regex identifiers do have a regex identifier test.
It's related to the file name and of course related to all other tests in this file.

@amadejpapez
Copy link
Collaborator

amadejpapez commented Oct 11, 2021

Moving this special tests out of regular regex tests file seems like a good idea to me. Tho I don't know what to name the file or if more files would be better?

@PabloLec
Copy link
Contributor Author

By the way, I guess we should also move test_check_keys_in_json from test_identifier.py to test_regex_formatting.py ?

I'll just move this one before merge

@bee-san bee-san merged commit 4ac82e6 into bee-san:main Oct 11, 2021
@PabloLec
Copy link
Contributor Author

PabloLec commented Oct 11, 2021

And while we are discussing tests files, I'm not sure about the usefulness of some tests in test_click.py. We do test that all regex work in test_regex_identifier.py and #202 plans to make it easier which is great. But there is a lot of repetition in test_click.py, every regex is also tested there. Either as an arg or from the fixture file.
I added some tests myself, as I seemed to be the way it was done but I'm not sure it is useful and pretty sure it could at least be simplified.
Any thoughts? Maybe I'm just missing something.

@amadejpapez
Copy link
Collaborator

When #202 gets merged with examples alongside in the database, we could maybe take those, write it to a file and run the file against pyWhat to verify that regex works if match is put in a file? But we also need to test boundaryless, so matches in between of something else..

@PabloLec
Copy link
Contributor Author

Let's discuss this on #199

@PabloLec PabloLec mentioned this pull request Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants