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

Fold the old integration tests into the new test suite. #2172

Closed
nateberkopec opened this issue Mar 10, 2020 · 6 comments · Fixed by #2347
Closed

Fold the old integration tests into the new test suite. #2172

nateberkopec opened this issue Mar 10, 2020 · 6 comments · Fixed by #2347

Comments

@nateberkopec
Copy link
Member

These tests are run with ruby test/shell/run.rb.

They should be replaced with new tests in the regular test suite, or removed if they are covering behavior already covered in the regular suite.

@harrylewis
Copy link
Contributor

Hey @nateberkopec! I've been digging into puma more recently, and would love to make a contribution. This seems like a good place for me to start, given that it is not feature-based.

I have managed to get the test suite to run locally, and will continue to review all the contributing guidelines and code of conduct. I see that there are already some test/*_integration_*.rb in the regular suite. I'm assuming the ones in question here should be migrated to a similar format?

Do you have any other advice?

@nateberkopec
Copy link
Member Author

Yup, you've got it right!

I think (hope?) that the three old tests should all be able to "fit" in the new format. It may be easier to open PRs one-by-one as you convert each test.

@harrylewis
Copy link
Contributor

Really happy to have contributed to this amazing project! I can continue to migrate the remaining 2 tests. I noticed a lot of test readability/speed improvements in #2241, which also make changes to the integration tests in question.

Do you want to defer to those changes, or should I go ahead and continue migrating these?

@wjordan
Copy link
Contributor

wjordan commented Apr 29, 2020

I would encourage you to continue! #2241 has a whole bunch of changes that are all going to need to be broken out into smaller PRs anyway, so I'll leave this particular change to you. I prefer the approach you took of folding the tests into the existing test class to take advantage of the existing helpers as well.

@sudheer-meka
Copy link

sudheer-meka commented Aug 27, 2020

Hey @nateberkopec I have created a PR 2346 one of the check is failing, can you please check why.

@sudheer-meka
Copy link

Hey @nateberkopec I have created a PR 2347 which moved the final integration test to regular suite but few of the checks are failing again, can you please check why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants