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
Various Test Improvements #8
Various Test Improvements #8
Conversation
This change also separates the different tests, rather than leaving them as a one test function that checks multiple conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like most of the changes you have done are really well integrated and you have fixed some bugs with the current codebase. I will do some personal testing with the changes of this PR on my local machine before merging with the main branch since this PR seems to be quite large.
I've been testing the changes and I have found some problems. Here are some of the most notable ones:
I'm willing to squash the commits as one once you fix and address the following issues and do some of the changes I suggested. I'm fine with some of the other minor issues such as the |
Yep, I realized this would happen after I slept- I replaced
The reason I did this was that the program was originally crashing when they were missing, which I ran into during testing (since my workstation isn't a server, I reconfigured it to a local directory and thought that would be sufficient.) I believed the most useful behavior was to have an easily-available version of the files. IMO, even with misconfiguration, the server should never panic, especially as a result of a runtime condition. I'll revert that for now, but consider fixing that in the future.
It does have a purpose- it makes it possible to move responsibility for reading the error files into the other function, which allows not returning a file. It'll be reverted as well.
See above. |
Yeah, this is intentional because I wanted to have a directory which would kind of work as a "development environment" so that you can test your changes and not have them apply when you rerun the server. This may be improved in the future, but it currently works. Just inform me when you commit the |
This commit (PR #8) modifies the testing framework and introduces a fuzzer to the codebase. Pre-squash commit messages can be found below. Replace fixed size buffer with a Vec<u8> This allows for using files of over 1024 bytes when needed. However, this commit still defaults to allocating a 1024 byte buffer for each request, and so does not likely reduce memory usage. Avoid panicking when error files don't exist Instead of panicking, the default files included in /media will be served. To accomplish this, these files are embedded into the binary at compile time. It is still possible to overwrite them by creating the expected files. Remove TestServer In its place, this commit modifies Server and some additional functions to be generic over their reader and writer, allowing unit tests to use the standard Server serve_request function. In addition, some bug fixes were made to allow the new tests to actually pass. Introduce fuzzing framework Fix bug found by fuzzing Revert "Avoid panicking when error files don't exist" This reverts commit 5c4e2f5. Copy media files to test directory Fix server hanging in real tests
By the way, I will fix the runtime panics in a future commit of mine (#9) |
@PeterPierinakos I've squashed and pushed. It should work now, sorry about breaking it the first time! Edit: Oops, tests failed because I forgot about a bug- the code to load |
Could you specify on what file and line the bug occurs in more detail so that I can take a look myself? By the way, I ran the tests on my local machine with your new changes and they worked just fine so I'm not sure why CI failed. |
In |
Oh, I thought you were talking about the tests breaking locally and not with CI, that's where my confusion came from. Could we possibly configure GitHub Actions to run the P.S. During writing this I just thought that we could write special tests specifically for CI that would use the |
This is the closest to what I expected. If you look in In short, I think the problem is that the configuration leaks into the |
I think 54378b7 should temporarily the CI issue. Because the configuration is a whole different topic and not very related to why this PR was initially created, I am now closing this issue as we have finally managed to integrate tests into the codebase. If any issue with this change persists after merging, feel free to create a new issue. Going to approve and merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All things that were needed to be fixed were fixed. Thanks for the effort put into addressing the issue and the PR!
This commit (PR #8) modifies the testing framework and introduces a fuzzer to the codebase. Pre-squash commit messages can be found below. Replace fixed size buffer with a Vec<u8> This allows for using files of over 1024 bytes when needed. However, this commit still defaults to allocating a 1024 byte buffer for each request, and so does not likely reduce memory usage. Avoid panicking when error files don't exist Instead of panicking, the default files included in /media will be served. To accomplish this, these files are embedded into the binary at compile time. It is still possible to overwrite them by creating the expected files. Remove TestServer In its place, this commit modifies Server and some additional functions to be generic over their reader and writer, allowing unit tests to use the standard Server serve_request function. In addition, some bug fixes were made to allow the new tests to actually pass. Introduce fuzzing framework Fix bug found by fuzzing Revert "Avoid panicking when error files don't exist" This reverts commit 5c4e2f5. Copy media files to test directory Fix server hanging in real tests
Various Test Improvements
Type of pull request
This PR makes changes to the test structure and introduces a basic fuzz test harness, as well as fixing a few panics that were tripped while writing the tests.
Overview of pull request
This PR follows through on the majority of #5 (comment). In particular, it moves the integration tests to become unit tests, completely removes the
TestServer
struct, adds a fuzz harness to detect panics that can be reached from outside, changes the hardcoded buffer sizes to be dynamically sized, and makes a few changes from what I assume to be the correct non-panicking behavior. In particular, I embedded the defaultmedia/
error pages into the binary, rather than simply panicking when they are not present- they are still possible to override (though there isn't a test case for that yet.) I decided to remove one of the existing test cases, because I don't think there's anything to do server-side for CORS checks, and I believe it would be easier to remove it than to reimplement the checks (another case where a future test would be good.) The base unit tests now cover roughly 65% of the codebase by the same metric as earlier, almost doubling previous coverage. As far as fuzzing, I ran it for roughly an hour with no crashes, and am currently reasonably confident that it is difficult for external requests to cause a panic at this point.I'm leaving this as separate commits right now to simplify reviewing it in parts, but would be happy to squash the changes into a single commit or multiple smaller commits if desired.
Future possibilities
I did not end up writing any integration tests for this PR. After making the changes I did, I think that going forward, the best course of action would be to move most of the server's unit tests back to being integration tests, but using a third party HTTP client over a socket rather than attempting to create a request by hand. The tests that remain as unit tests should be the ones that e.g. test an empty body and providing broken unicode. I've spent long enough not submitting this PR that I decided to leave it as is, but there are also some additional tests that would be good to write:
If desired, I can redo the hack in the server unit tests to use
once_cell
, or even just replace it with a static junk path (I don't think it's ever used, so it would be fine to hardcode it to "nonexistent").