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

Adding a unit test suite #21

Merged
merged 18 commits into from
May 29, 2020
Merged

Conversation

GenevieveBuckley
Copy link
Contributor

@GenevieveBuckley GenevieveBuckley commented May 27, 2020

Adds a unit test suite
Closes #19

This PR:

  • Adds a tox.ini configuration file
  • Adds a GitHub Actions config to run CI tests
  • Tests that pyproject.toml can be parsed without error
  • Tests that there are no flake8 errors raised on any of the generated python code
  • Tests that all the generated python files can be compiled using py_compile
  • Tests that the filename tree structure of the generated app matches what we expect

STILL TO DO:

  • Expand to test all GUI frameworks (PySide, PursuedPyBear, etc), not just the default Done!
  • Replace os.path functions with pathlib where appropriate

I had assumed adding support for all the GUI frameworks would be as easy as adding a couple more fixtures and @pytest.mark.parametrize to all the tests -- but it seems like you can't pass fixtures into parametrize like that. Any suggestions?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start. I've flagged a few cleanups (mostly stylistic variations, or conventions that BeeWare projects have that may not be obvious).

As for the parametrization question; You can't use a fixture as a parameter (AFAIK), but you can pass parameters to a fixture:

@pytest.fixture
def tester(tester_arg):
    return tester_arg * 3 + 1

@pytest.mark.parametrize('tester_arg', [0, 1, 2, 3])
def test_mytest(tester):
    assert tester % 2 == 0

So - if you defined a fixture for a "project", and passed in the GUI library (and maybe the license as well) as an argument to the template, you can then parameterize the tests with the parameterized project as a fixture. Since there are multiple tests that need to be parametrized in this way, it might be helpful to pull out the "list of test configurations" as a constant that can be re-used between the individual tests.

.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tests/test_app_template.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@GenevieveBuckley
Copy link
Contributor Author

I've addressed all the points you made, thank you for the review!

Note that:

  • I didn't add licenses other than the default to our test cases, primarily because all that changes is the contents of a single text file & there is no test checking that content currently. So as things stand now, it's a little meaningless.
  • The fixture has a function-scope. Maybe that's slightly inefficient but on the whole it's fine because the 15 tests run in under 13 seconds. (That does bug me a little, I wanted to make it class scoped then pack the three tests into a single class & parameterize that. But I couldn't get that working nicely with the argument passing,)

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for the contribution!

@freakboy3742 freakboy3742 merged commit 281b2e0 into beeware:v0.3 May 29, 2020
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.

2 participants