search

urinieto / msaf

Music Structure Analysis Framework
MIT License
490 stars 78 forks source link

Add GitHub Actions test automation on PRs #128

Closed carlthome closed 1 year ago

carlthome commented 1 year ago

What?

Introduce automated testing on PRs by GitHub Actions.

Why?

To make PR reviews easier by checking that unit tests pass automatically on fresh installations of MSAF.

How?

  1. Introduce a workflow YAML for GitHub Actions that triggers on pull request changes or pushes to the master branch.
  2. Headbutting with dependencies (e.g. installing a master commit of mir_eval directly from its GitHub.com repo, since the package has not been released to PyPI recently, and was broken by recent NumPy deprecations).
urinieto commented 1 year ago

Nice! Can you add a link to the GitHub Actions test runs of this PR? (I'm not too familiar with how they work and it'd be great to see them in action before merging).

carlthome commented 1 year ago

Nice! Can you add a link to the GitHub Actions test runs of this PR? (I'm not too familiar with how they work and it'd be great to see them in action before merging).

Here's an example of how it will look from my fork: https://github.com/carlthome/msaf/actions/runs/5201400055

Think it's quite nice now, as a first step for easier reviewing.

Would need to release a newer mir_eval though to simplify the workflow a bit (https://github.com/urinieto/msaf/pull/128/files#diff-245392b692a50c38ecab4381b118862db514035c10983f3bd4f4b7f1f4be4692R39).

Also had to replace nosetests in favor of pytest for newer Python releases, but pytest doesn't like the yield based tests so some of them are actually skipped right now (https://github.com/carlthome/msaf/actions/runs/5201400055/jobs/9381491368#step:8:32).

I think this is better than nothing though. Propose adding this to master, and then improve test coverage in subsequent PRs.

urinieto commented 1 year ago

Looking great, thanks! I created a new issue regarding the "yield" in PyTest here: https://github.com/urinieto/msaf/issues/129