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 bin/test to detect and trigger unit tests #982

Closed
wants to merge 2 commits into from

Conversation

vincetse
Copy link

Background
Most Heroku buildpacks have a bin/test script that runs application unit tests (e.g the Ruby & PHP buildpacks), but it appears that the Python buildpack is currently missing one.

Implementation
I have taken inspiration from the PHP buildpack's test script, and added support for detecting the unit test framework used by grepping through Pipfile or requirements.txt.

So far, I have added support for Pytest, Nose, Nose2, Python's unittest module, as well as Django's unit functionality. I have also added the ability to define a custom unit test command by defining the command as the value of the UNIT_TEST_COMMAND environment variable so that the user has some flexibility.

Buildpack unit tests have also been added to confirm that the functionality is correct.

In Practice
This functionality probably isn't used on the Heroku platform since it is missing, but it is useful when using the buildpack outside of Heroku, most noticeably with Herokuish (i.e. it addresses the Python part of gliderlabs/herokuish#349).

@vincetse vincetse requested a review from a team as a code owner May 28, 2020 11:42
@edmorley edmorley changed the base branch from master to main August 3, 2020 13:29
@ahrberg
Copy link

ahrberg commented Nov 3, 2020

Will this be merged? Very much appreciated.

@vincetse
Copy link
Author

vincetse commented Nov 7, 2020

Rebased my branch against main since there is some interest from others. The PR Travis tests will probably never pass since I don't have your HEROKU_API_USER and HEROKU_API_KEY secrets. For your consideration. Thank you! :)

@vincetse
Copy link
Author

Hi there, @edmorley. Can you give a up or down vote here? Happy to make adjustments, or happy to close this out if you don't think this aligns with Heroku's vision. Hate to see stuff languish in your backlog if it's useful.

@edmorley
Copy link
Member

edmorley commented Apr 8, 2021

@vincetse Sorry for the delayed reply. This change falls into that difficult category of being something that does seem useful (so not a PR that would be closed straight away as wontfix), but is quite low down the priority list (given it only affects Heroku CI, there is an easy workaround of adding an explicit test script to app.json, and no customers have requested it) so hard to justify the time to work through the details (implementation choices, UX, docs, inevitable follow-up feature requests/support tickets etc).

For many of the Python projects I've used in the past, we've had to specify additional CI specific test suite options as arguments to the test run, so I'm also not sure how many people will be able to use this automatic behaviour out of the box.

In general I also think that being explicit about what command is run (ie: specifying it in app.json) is more in keeping with the zen of Python ("explicit is better than implicit"), and also easy for someone new to a project to figure out how the tests run. For example the first thing I tend to do when contributing to a new open source (or even internal work) project (after checking out the code locally so I can work on it), is to look at the Travis/Circle/... CI config to see how the tests are run (if there's no other obvious documentation). If the test detection happens magically, this isn't possible.

Lastly, I'm about to start working on a CNB (Cloud Native Buildpack) version of this buildpack, which will eventually replace this buildpack. The spec/API for test running for CNBs has not yet been created (buildpacks/community#62 (comment)) so I'd like to avoid adding an implementation for classic buildpacks, only to have to change it later.

As such I'll close this out for now (just to save keeping a PR open for months), but happy to revisit in the future CNB world :-)

@edmorley
Copy link
Member

edmorley commented Apr 8, 2021

There was an existing issue open for this (#924) - I'll leave that open as a reminder / somewhere for future discussions to take place.

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.

3 participants