Add integration test #81
Conversation
This reverts commit 8302b7f.
|
The problem with this is, it pollutes the dependencies and as I said before it brings dependencies that may not be in lock-step on what rustls we are developing - any dependency must IMO not be in conflict and not duplicate / confuse. Please see the openssl reference from validation directory and there isn't really more than we need to do - bad handling of SSL protocol itself is in the scope of rustls and our scope I believe is just to make sure ciphersuites etc work which we do. If we need to address some primitive level concerns then we should isolate or "minimise" those test cases and bring them local and without external dependencies that pollute dependency tree and make issues hard to track down given multiple failing / conflicting / confusing parts that not only duplicate the dependency tree but are not in sync what is being developed against. Also external services have a habit of disappearing and being flaky - Integration tests IMO must not be dependant on external service or libraries that have dependency conflicts as they do now e.g. when we were between 0.22 and 0.23. Plus adding cargo nextest is something we are not using atm - typically it is a good idea to use minimal dependencies as to development enviromment itself and be tool neutral. |
|
@pinkforest I do understand how the tests should be kept lean and mean but we need to be more comprehensive if we want this to be really useful with meaningful real world impacts. How about we put basic (likely bare minimal) integration tests that is in lock step of the rustls version, and then we have a more complete integration test suite with real life examples, that can be triggered on demand once we found all the dependencies are clean. Simply put, we could isolate the basic and complete integration tests, and the CI always run the basic integration tests while we can use I chose nextest because it boosted test time 3x in my local dev environment with useful indicators and more concise result. And it is also a slick peek of what cargo test would be in the future (it is the next generation cargo test executor, which would be merged after all in some day) |
|
Another option would be making a separate |
Brilliant! Just did it in case you don't know. But seems like there is an apparent problem. Cargo.lock is still garbled up. But we can try to at least mitigate it with dev-dependencies... |
I think we should put the integration tests right into the test pipeline. This includes the previously included real world test against BadSSL (despite BadSSL itself is outdated), some popular websites and I also added a simple Tower+Hyper based server-client roundtrip test.
I have also changed the test engine to cargo-nextest, and it should give us a much faster and nicer interface to quickly respond to the test results. It is not tested locally with act yet but I think it should work out of the box.