Skip to content
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

Add Bandit to tests in /integration #5707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mtrudel
Copy link
Member

@mtrudel mtrudel commented Jan 30, 2024

No description provided.

@mtrudel
Copy link
Member Author

mtrudel commented Jan 31, 2024

These tests run quite a bit slower than previously because we're starting / stopping the underlying server via a setup block instead of a setup_all (ie: we're doing it for each test). Cowboy seems to have a stock 5 second waiting period at the end of every test as a result (previously, it only waited once, at the end of the entire file).

We don't really need to be doing this this way, but fixing it properly will be a bit more surgery than what's here. I'll get it fixed up.

@mtrudel mtrudel marked this pull request as draft February 2, 2024 03:49
@mtrudel
Copy link
Member Author

mtrudel commented Nov 14, 2024

Finally had a chance to look at the Cowboy slowness. Didn't / couldn't solve the underlying issue with Cowboy taking 5 seconds to terminate at the end of each test set. Moreover, we need to keep the Cowboy startup in a setup_all call (otherwise it gets started/stopped on every test, which adds 5s to every test instead of just to the entire test suite).

As a result, the structure of these tests is a bit oddball, with the tests themselves macro'd into discrete Bandit/Cowboy test modules. The vast majority of the noise in this PR is just indenting to accommodate the macro-ing of the tests.

Would love a better way to do this; if anyone has ideas I'm all ears.

Also, there's one failing setup in test/phoenix/integration/websocket_channels_test.exs that I can't figure out how to resolve. Hopefully more eyeballs make it transparent.

@mtrudel mtrudel marked this pull request as ready for review November 14, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant