search

PeterPierinakos / vanilla-rustlang-server

Simple, minimal and open source static web server that uses no dependencies.
Apache License 2.0
39 stars 1 forks source link

Various Test Improvements #8

Closed zombiepigdragon closed 2 years ago

zombiepigdragon commented 2 years ago

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 https://github.com/PeterPierinakos/vanilla-rustlang-server/issues/5#issuecomment-1164972312. 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 default media/ 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:

PeterPierinakos commented 2 years ago

I've been testing the changes and I have found some problems. Here are some of the most notable ones:

  1. (This first one is your highest priority.) The server doesn't respond to any of my requests when I run it from your fork and I try to send a request to the server using curl http://localhost:80. Doesn't matter if I run as root or not. This seems to be caused by read_stream and some of the changes you made to it.
  2. At line 21 of file.rs, you try to access the media files which shouldn't be accessed by the program. You can imagine the media files as "stage 1" of implementing the static files because after you put them there you are supposed to run ./setup.sh which will copy all of them to /var/www/static/ (the default static path), so you are supposed to access them via that. The changes made to media/ should be local.
  3. Some of the changes you have done are redundant. For instance, changing the match statement to an if ... else ... statement inside response.rs is unnecessary, so you should probably revert that change.
  4. read_error_file hard checks for if the filename is any of the 4 error pages. The logic is already handled by the beginning of create_file_response, so I believe that this change is also redundant and should be reverted to the function's original state.

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 Box::leak in the fuzz_serve_request, but the ones I mentioned above should be fixed before merging.

zombiepigdragon commented 2 years ago

(This first one is your highest priority.) The server doesn't respond to any of my requests when I run it from your fork and I try to send a request to the server using curl http://localhost:80. Doesn't matter if I run as root or not. This seems to be caused by read_stream and some of the changes you made to it.

Yep, I realized this would happen after I slept- I replaced read with read_to_end, which works fine in tests and avoids possible truncation, but isn't actually correct because HTTP doesn't close the stream until the reply is sent- I'll update that once I'm done typing here.

At line 21 of file.rs, you try to access the media files which shouldn't be accessed by the program. You can imagine the media files as "stage 1" of implementing the static files because after you put them there you are supposed to run ./setup.sh which will copy all of them to /var/www/static/ (the default static path), so you are supposed to access them via that. The changes made to media/ should be local.

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.

Some of the changes you have done are redundant. For instance, changing the match statement to an if ... else ... statement inside response.rs is unnecessary, so you should probably revert that change.

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.

read_error_file hard checks for if the filename is any of the 4 error pages. The logic is already handled by the beginning of create_file_response, so I believe that this change is also redundant and should be reverted to the function's original state.

See above.

PeterPierinakos commented 2 years ago

The reason I did this was that the program was originally crashing when they were missing, which I ran into during testing

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 read_to_end bug fix and I'll merge to main 👍

PeterPierinakos commented 2 years ago

By the way, I will fix the runtime panics in a future commit of mine (#9)

zombiepigdragon commented 2 years ago

@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 400.html and its counterparts relies on configuration.rs, so it doesn't work in combination with the tests. I'm not sure about the cleanest fix, since it would be a pretty big change to just pass the configured path all the way down the call chain. Thoughts?

PeterPierinakos commented 2 years ago

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.

zombiepigdragon commented 2 years ago

In /src/util/file.rs line 16, the current configured ABSOLUTE_STATIC_CONTENT_PATH is used. If there are files in that directory, the tests will pass, but without setting up /var/www/static the aforementioned panic due to missing files will trip. CI doesn't use that directory, so the tests fail when it can't find the files.

PeterPierinakos commented 2 years ago

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 setup.sh bash script before executing the tests since I'm guessing it's running on top of a Linux distribution? Or do we have to implement your suggestion and entirely remove /var/www/static and just use the media/ directory which isn't outside of the repository as previously mentioned? The laziest but also the easiest way to deal with this would be the latter I recommended.

P.S. During writing this I just thought that we could write special tests specifically for CI that would use the media/ directory and wouldn't be used by normal end users.

zombiepigdragon commented 2 years ago

P.S. During writing this I just thought that we could write special tests specifically for CI that would use the media/ directory and wouldn't be used by normal end users.

This is the closest to what I expected. If you look in src/structs/server.rs line 276, I was hoping it would use tests/static and therefore avoid crashing. The problem here is that user configuration affects tests, which is usually not a good thing. /var/www/static or similar definitely should continue to be the default path, it would be really weird to default to having to install the site within the source code. Running setup.sh before launching tests is a working band-aid if you think that is the best way to go, but it doesn't always help. On my machine, the local tests failed after I reset configuration.rs (I had it set to the test files directory and a rootless port for debugging ease).

In short, I think the problem is that the configuration leaks into the find_file function, and it should somehow avoid being directly dependent upon the installed settings.

PeterPierinakos commented 2 years ago

I think 54378b74433972ffbc07b874cf4cc0ebc1fb4229 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.