PaulMcInnis / JobFunnel

Scrape job websites into a single spreadsheet with no duplicates.
MIT License
1.78k stars 210 forks source link

Added tests for validate on search.py #121

Closed thebigG closed 3 years ago

thebigG commented 3 years ago

Tests for validate method on search.py

Description

Hi all,

Hope you are all doing well.

Noticed that we were not testing the validate method on search.py, so I added all those tests.

Context of change

Please add options that are relevant and mark any boxes that apply.

Type of change

Please mark any boxes that apply.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Checklist:

Please mark any boxes that have been completed.

thebigG commented 3 years ago

Nice job! Seems good to check what kind of search configurations should be valid.

Looking at the file it seems that we can further abstract these tests. In essence, we provide a search configuration and check if JobFunnel raises an AssertionError with a certain message. This leads me to think that we could potentially define a list of search configurations and expected error message. After we just need one test function, say, test_search_config_validate_invalid, and provide the list of search configurations.

@markkvdb I'm always hesitant to doing this when writing tests. Because usually if you pass a list with all permutations of configurations to a single test, you might have to have if statements in there. And that can mess with the readability of what should be a very simple test. Though I know sometimes that can be done in a somewhat clean way, but in this case I suspect we will have logic that might add unnecessary complexity to what could be a very simple test.

I try my best to keep tests simple and dumb, especially unit tests. If the tests are simple and dumb, points of failure are very easy to investigate.

I know this approach can be very slow, but I hope the reasoning behind it makes sense.

codecov-io commented 3 years ago

Codecov Report

Merging #121 into master will increase coverage by 0.55%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   35.74%   36.30%   +0.55%     
==========================================
  Files          22       22              
  Lines        1449     1449              
==========================================
+ Hits          518      526       +8     
+ Misses        931      923       -8     
Impacted Files Coverage Δ
jobfunnel/config/search.py 100.00% <0.00%> (+23.52%) :arrow_up:

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 cf65ca7...b122bee. Read the comment docs.