test: restructure internal config cases and fixtures
#1250
Conversation
G-Rath
force-pushed
the
config/improve-tests
branch
from
9d4a79a
to
b693390
Compare
| @@ -0,0 +1 @@ | |||
| LoadPath = "a/b/c" | |||
There was a problem hiding this comment.
note: technically due to the tags on Config this will get read, but we always override it so isn't not possible for this to work (which is what we want)
however if someone were to refactor tryLoadConfig (like what I've done in #1248), then this could start getting loaded - hence, I've added this test to safeguard, with the intent to address the actual (currently theoretical) bug in a follow-up
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1250 +/- ##
==========================================
- Coverage 67.96% 67.96% -0.01%
==========================================
Files 174 174
Lines 16848 16848
==========================================
- Hits 11451 11450 -1
- Misses 4764 4765 +1
Partials 633 633 ☔ View full report in Codecov by Sentry. |
pkg/config/config_internal_test.go
Outdated
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "target is file in directory with config", |
There was a problem hiding this comment.
maybe I miss something - should this be directory without config? there isn't osv-scanner.toml in innerFolder
There was a problem hiding this comment.
🤦 I had it written without that the first time, but then got confused with the tryLoadConfig tests and added it - thanks for catching this!
There was a problem hiding this comment.
so now there are two target is file in directory tests - any difference between two tests?
There was a problem hiding this comment.
there were always two tests like this - I've just ported them across. I think technically they're different because one has a config file in them and the other doesn't, but the original test didn't make it clear if that was a part of the test or just a coincidence.
I wasn't that fussed because these tests are very fast to run so I didn't have a huge issue with retaining a potentially duplicated test here (given it's not a total duplicate from a file system POV even if that's not super revertant to the implementation)
There was a problem hiding this comment.
I think it's better to state the difference in the test names - maybe @another-rex knows why we have these tests.
These tests are so old that they predate the repository itself and are not giving the best coverage of what they say they're testing so I've refactored them so that: