Test writable loader with apache config files #74
Conversation
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
=========================================
Coverage ? 81.55%
=========================================
Files ? 7
Lines ? 759
Branches ? 0
=========================================
Hits ? 619
Misses ? 140
Partials ? 0
Continue to review full report at Codecov.
|
etingof
left a comment
etingof
left a comment
There was a problem hiding this comment.
I started testing apacheconfig against these files-- it might be good to check in a bunch of these files, to prevent future regressions. Let me know what you think.
I concur, having real-world samples could be useful for catching regressions.
If you anticipate maintaining a large collection of sample files, may be it would make sense to push them all into a separate package just for testing (e.g. apacheconfig-tests)...?
I am good with both approaches.
| @@ -0,0 +1,6 @@ | |||
| # Modules required to parse these conf files: | |||
There was a problem hiding this comment.
Do we really need these modules and this README file to parse the configs?
| for filename in os.listdir(directory): | ||
| text = "" | ||
| filepath = os.path.join(directory, filename) | ||
| if os.path.isdir(filepath): |
There was a problem hiding this comment.
nit: may be ensure that it's a file and skip if it is not?
| def _test_files(self, directory, perform_test, expected_errors=0): | ||
| errors = [] | ||
| for filename in os.listdir(directory): | ||
| text = "" |
| self.assertEqual(len(errors), expected_errors) | ||
|
|
||
| def testLexFiles(self): | ||
| samples_dir = os.path.join( |
There was a problem hiding this comment.
Would it make sense to move samples_dir initialization to setUp?
Yeah, we chatted about this briefly as well (breaking out our test files from Certbot). For the time being, I think this is alright, but if we find ourselves updating this set of files frequently, it would definitely make more sense to pull it out into an independent testing package that both Certbot and apacheconfig can depend on. |
3 participants
Depends on #73. Check out the last two commits for the full diff.
In Certbot, we have a lot of sample config files that we test against. Here's a bunch from issues in the past:
https://github.com/certbot/certbot/tree/5b45d0742ad51bb9ad93dd61977b5b2d4bcc4319/certbot-apache/certbot_apache/tests/apache-conf-files
And we also test against the default system apache configurations on different OSes:
https://github.com/certbot/certbot/tree/5b45d0742ad51bb9ad93dd61977b5b2d4bcc4319/certbot-apache/certbot_apache/tests/testdata
I started testing
apacheconfigagainst these files-- it might be good to check in a bunch of these files, to prevent future regressions. Let me know what you think.This is a draft pull request for now! Going to add some more test configs.
EDIT: Woah! this is a huge diff. Most of it is just really long config files. The only piece of code added in this PR should be a new integration test at
tests/integration/test_apache_samples.py.