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

refactor tests/test_regex_identifier.py #202

Merged
merged 30 commits into from
Oct 16, 2021
Merged

refactor tests/test_regex_identifier.py #202

merged 30 commits into from
Oct 16, 2021

Conversation

jyooru
Copy link
Contributor

@jyooru jyooru commented Oct 10, 2021

⚠ Pull Requests not made with this template will be automatically closed 🔥

Prerequisites

Why do we need this pull request?

Currently, the tests files are quite long. I've attempted moving the test data into regex.json and using pytest.mark.parmaterize to "generate" tests using the test data now in regex.json. This makes it much easier to maintain test_regex_identifier.py as to change all _assert_match_first_item tests, you only have to change one test:

@pytest.mark.parametrize(
    "name,match",
    [
        (regex["Name"], match)
        for regex in database
        for match in regex.get("Matches", [])
    ],
)
def test_regex_match(name: str, match: str):
    res = r.check([match])
    _assert_match_first_item(name, res)

What GitHub issues does this fix?

Related to #199
Related to #195

Contributions to this PR are welcome!

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2021

Codecov Report

Merging #202 (c7970e4) into main (2589780) will decrease coverage by 2.18%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
- Coverage   94.50%   92.32%   -2.19%     
==========================================
  Files          14       14              
  Lines        1729     1198     -531     
==========================================
- Hits         1634     1106     -528     
+ Misses         95       92       -3     
Impacted Files Coverage Δ
tests/test_regex_database.py 87.50% <55.55%> (ø)
tests/test_regex_identifier.py 96.77% <95.23%> (-1.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2589780...c7970e4. Read the comment docs.

@ghost ghost mentioned this pull request Oct 10, 2021
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.

Can you change "Matches" to "Examples"?

@jyooru
Copy link
Contributor Author

jyooru commented Oct 10, 2021

Can you change "Matches" to "Examples"?

Yeah, I'm planing on also adding some strings that should not be matched. Does something like this sound alright?

{
  "Name": "Email Address",
  ...
  "Examples": {
    "Valid": ["firstname+lastname@example.com"],
    "Invalid": ["email@example@example.com"]
  }
}
def test_email4():
    res = r.check(["email@example@example.com"])
    assert "Email Address" not in res

@ghost
Copy link

ghost commented Oct 10, 2021

Can you change "Matches" to "Examples"?

Yeah, I'm planing on also adding some strings that should not be matched. Does something like this sound alright?

{

  "Name": "Email Address",

  ...

  "Examples": {

    "Valid": ["firstname+lastname@example.com"],

    "Invalid": ["email@example@example.com"]

  }

}
def test_email4():

    res = r.check(["email@example@example.com"])

    assert "Email Address" not in res

Amazing

@bee-san
Copy link
Owner

bee-san commented Oct 10, 2021

This is great!! Makes it way easier for people to test their code, and it allows people in the future to see examples of things :D

happy to merge this, 1 question -- why is code coverage down? 🤔 other than that, it looks good to me :D !!! 🥳

@jyooru
Copy link
Contributor Author

jyooru commented Oct 10, 2021

This is great!! Makes it way easier for people to test their code, and it allows people in the future to see examples of things :D

happy to merge this, 1 question -- why is code coverage down? thinking other than that, it looks good to me :D !!! partying_face

Thanks!

Code coverage is down due to amount of lines of code removed, as pytest is practically just rerunning the function for each match. JSON does not contribute to coverage either. I will be checking that there is the same amount of test cases before and after, that way I know I haven't left anything out accidentally.

@PabloLec PabloLec mentioned this pull request Oct 11, 2021
1 task
@jyooru jyooru marked this pull request as ready for review October 16, 2021 11:38
@jyooru
Copy link
Contributor Author

jyooru commented Oct 16, 2021

Please squash!

@bee-san
Copy link
Owner

bee-san commented Oct 16, 2021

Can you change the codecov stuff slightly so it passes CI? :)

@bee-san bee-san requested a review from a user October 16, 2021 12:07
@jyooru jyooru changed the title parametrize tests refactor tests/test_regex_identifier.py Oct 16, 2021
@bee-san bee-san enabled auto-merge (squash) October 16, 2021 12:07
@bee-san bee-san disabled auto-merge October 16, 2021 12:54
@bee-san bee-san merged commit dd077aa into bee-san:main Oct 16, 2021
@jyooru jyooru deleted the test/parametrize branch October 16, 2021 13:09
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.

3 participants