diff --git a/.circleci/config.yml b/.circleci/config.yml index d4bcd86a5..5a0aa7238 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,4 +1,4 @@ -# Python CircleCI 2.0 configuration file +# Python CircleCI 2.1 configuration file # # Check https://circleci.com/docs/2.0/language-python/ for more details # diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 314bd9b46..3116ab1ac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,13 +11,16 @@ Here are some [instructions][link_signupinstructions]. Already know what you're looking for in this guide? Jump to the following sections: * [Joining the conversation](#joining-the-conversation) - * [Monthly developer calls](#monthly-developer-calls) * [Contributing small documentation changes](#contributing-small-documentation-changes) * [Contributing through Github](#contributing-through-github) * [Understanding issues, milestones, and project boards](#understanding-issues-milestones-and-project-boards) +* [Installing in editable mode](#3-Run-the-developer-setup) * [Making a change](#making-a-change) +* [Testing your change](#5-Test-your-changes) +* [Viewing Documentation Locally](#Changes-to-documentation) * [Structuring contributions](#style-guide) -* [Recognizing contributors](#recognizing-contributions) +* [Recognizing contributors](#Recognizing-contributors) +* [Monthly calls and testing guidelines][link_developing_rtd] Don't know where to get started? Read [Joining the conversation](#joining-the-conversation) and pop into @@ -33,15 +36,6 @@ We also maintain a [gitter chat room][link_gitter] for more informal conversatio There is significant cross-talk between these two spaces, and we look forward to hearing from you in either venue! As a reminder, we expect all contributions to `tedana` to adhere to our [code of conduct][link_coc]. -### Monthly developer calls - -We run monthly developer calls via Zoom. -You can see the schedule via the `tedana` [google calendar](https://calendar.google.com/calendar/embed?src=pl6vb4t9fck3k6mdo2mok53iss%40group.calendar.google.com). -An agenda will be circulated in the gitter channel in advance of the meeting. - -Everyone is welcome. -We look forward to meeting you there :hibiscus: - ## Contributing small documentation changes If you are new to GitHub and just have a small documentation change recommendation, please submit it to [our e-mail address](mailto:tedana.devs@gmail.com) @@ -92,11 +86,6 @@ towards ``tedana``'s shared vision. We might have just missed it, or we might not (yet) see how it aligns with the overall project structure. These conversations are important to have, and we are excited to hear your perspective! -* The **project board** is an automated [Kanban board][link_kanban] to keep track of what is currently underway -(in progress), what has been completed (done), and what remains to be done for a specific release. -The ``tedana`` maintainers use this board to keep an eye on how tasks are progressing week by week. - - ### Issue labels The current list of labels are [here][link_labels] and include: @@ -151,30 +140,66 @@ Once you've run this, your repository should be set for most changes (i.e., you ### 4. Make the changes you've discussed -Try to keep the changes focused to the issue. We've found that working on a [new branch][link_branches] for each issue makes it easier to keep your changes targeted. +Try to keep the changes focused to the issue. +We've found that working on a [new branch][link_branches] for each issue makes it easier to keep your changes targeted. +Using a new branch allows you to follow the standard GitHub workflow when making changes. +[This guide][link_gitworkflow] provides a useful overview for this workflow. +Before making a new branch, make sure your master is up to date with the following commands: -Using a new branch allows you to follow the standard "fork/branch/commit/pull-request/merge" GitHub workflow when making changes. [This guide][link_gitworkflow] provides a useful overview for this workflow. +``` +git checkout master +git fetch upstream master +git merge upstream/master +``` + +Then, make your new branch. + +``` +git checkout -b MYBRANCH +``` + +Please make sure to review the `tedana` [style conventions](#style-guide) and test your changes. + +If you are new to ``git`` and would like to work in a graphical user interface (GUI), there are several GUI git clients that you may find helpful, such as +- [GitKraken][link_git_kraken] +- [GitHub Desktop][link_github_desktop] +- [SourceTree][link_source_tree] -Before creating your pull request, please make sure to review the `tedana` [style conventions](#style-guide). ### 5. Test your changes -#### Changes to code +You can run style checks by running the following: +``` +flake8 $TEDANADIR/tedana +``` -For changes to the codebase, we suggest using our development Docker container which will run all the necessary checks and tests to ensure your code is ready to be merged into `tedana`! -(This does require that you have a local install of [Docker](https://www.docker.com/products/docker-desktop).) -You can run all the checks with: +and unit/integration tests by running `pytest` (more details below). +If you know a file will test your change, you can run only that test (see "One test file only" below). +Alternatively, running all unit tests is relatively quick and should be fairly comprehensive. +Running all `pytest` tests will be useful for pre-pushing checks. +Regardless, when you open a Pull Request, we use CircleCI to run all unit and integration tests. +All tests; final checks before pushing +``` +pytest $TEDANADIR/tedana/tests +``` +Unit tests and linting only ``` -docker run --tty --rm -v ${PWD}:/tedana tedana/tedana-dev:latest run_all_tests +pytest --skipintegration $TEDANADIR/tedana/tests +``` +One test file only +``` +pytest $TEDANADIR/tedana/tests/test_file.py +``` +Test one function in a file +``` +pytest -k my_function $TEDANADIR/tedana/tests/test_file.py ``` from within your local `tedana` repository. -(**N.B.** It is possible that, depending on your Docker setup, you may need to increase the amount of memory available to Docker in order to run the `tedana` test suite. -You can either do this permanently by editing your Docker settings or temporarily by adding `--memory=4g` to the above `docker run` command.) - -This will print out a number of different status update messages as the tests run, but if you see `"FINISHED RUNNING ALL TESTS! GREAT SUCCESS"` then it means everything finished succesfully. -If not, there should be some helpful outputs that specify which tests failed. +The test run will indicate the number of passes and failures. +Most often, the failures give enough information to determine the cause; if not, you can +refer to the [pytest documentation][link_pytest] for more details on the failure. #### Changes to documentation @@ -189,11 +214,23 @@ from the `docs` directory in your local `tedana` repository. You should then be When opening the pull request, we ask that you follow some [specific conventions](#pull-requests). We outline these below. After you have submitted the pull request, a member of the development team will review your changes to confirm that they can be merged into the main code base. +When you have two approving reviewers and all tests are passing, your pull request may be merged. -After successful merging of the pull request, remember to [keep your fork up to date][link_updateupstreamwiki] with the master `tedana` repository and to delete the branch on your fork that was used for the merged pull request. ### Pull Requests +To push your changes to your remote, use + +``` +git push -u origin MYBRANCH +``` + +and GitHub will respond by giving you a link to open a pull request to +ME-ICA/tedana. +Once you have pushed changes to the repository, please do not use commands such as rebase and +amend, as they will rewrite your history and make it difficult for developers to work with you on +your pull request. You can read more about that [here][link_git_rewriting]. + To improve understanding pull requests "at a glance", we encourage the use of several standardized tags. When opening a pull request, please use at least one of the following prefixes: @@ -202,20 +239,32 @@ When opening a pull request, please use at least one of the following prefixes: * **[ENH]** for enhancements * **[FIX]** for bug fixes * **[REF]** for refactoring existing code -* **[STY]** for stylistic changes * **[TST]** for new or updated tests, and -* **[WIP]** for changes which are not yet ready to be merged +* **[MAINT]** for maintenance of code + +You can also combine the tags above, for example if you are updating both a test and +the documentation: **[TST, DOC]**. Pull requests should be submitted early and often! -If your pull request is not yet ready to be merged, please also include the **[WIP]** prefix. +If your pull request is not yet ready to be merged, please use [draft PRs][link_draftpr] This tells the development team that your pull request is a "work-in-progress", and that you plan to continue working on it. -We request that you do not use the Draft PR feature at this time, -as it interferes with our Continuous Integration tool, Travis. - -You can also combine the tags above, for example if you are updating both a test and -the documentation: **[TST, DOC]**. -If you're still working on the pull request that prefix would be **[WIP, TST, DOC]**. +If no comments or commits occur on an open Pull Request, stale-bot will comment in order to remind +both you and the maintainers that the pull request is open. +If at this time you are awaiting a developer response, please ping them to remind them. +If you are no longer interested in working on the pull request, let us know and we will ask to +continue working on your branch. +Thanks for contributing! + +### Pull Request Checklist (For Fastest Review): +- [ ] Check that all tests are passing ("All tests passsed") +- [ ] Make sure you have docstrings for any new functions +- [ ] Make sure that docstrings are updated for edited functions +- [ ] Make sure you note any issues that will be closed by your PR +- [ ] Take a look at the automatically generated readthedocs for your PR (Show all checks -> continuous-documentation/readthedocs -> Details) + +### Comprehensive Developer Guide +For additional, in-depth information on contributing to `tedana`, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. ## Style Guide @@ -270,7 +319,7 @@ You're awesome. :wave::smiley: [writing_formatting_github]: https://help.github.com/articles/getting-started-with-writing-and-formatting-on-github [markdown]: https://daringfireball.net/projects/markdown [rick_roll]: https://www.youtube.com/watch?v=dQw4w9WgXcQ -[restructuredtext]: http://docutils.sourceforge.net/rst.html#user-documentation +[restructuredtext]: http://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html [sphinx]: http://www.sphinx-doc.org/en/master/index.html [readthedocs]: https://docs.readthedocs.io/en/latest/index.html @@ -289,6 +338,7 @@ You're awesome. :wave::smiley: [link_kanban]: https://en.wikipedia.org/wiki/Kanban_board [link_pullrequest]: https://help.github.com/articles/creating-a-pull-request/ +[link_draftpr]: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-requests#draft-pull-requests [link_fork]: https://help.github.com/articles/fork-a-repo/ [link_pushpullblog]: https://www.igvita.com/2011/12/19/dont-push-your-pull-requests/ [link_updateupstreamwiki]: https://help.github.com/articles/syncing-a-fork/ @@ -306,3 +356,10 @@ You're awesome. :wave::smiley: [link_all-contributors-bot]: https://allcontributors.org/docs/en/bot/overview [link_all-contributors-bot-usage]: https://allcontributors.org/docs/en/bot/usage [link_stemmrolemodels]: https://github.com/KirstieJane/STEMMRoleModels +[link_pytest]: https://docs.pytest.org/en/latest/usage.html +[link_developing_rtd]: https://tedana.readthedocs.io/en/latest/developing.html + +[link_git_kraken]: https://www.gitkraken.com/ +[link_github_desktop]: https://desktop.github.com/ +[link_source_tree]: https://desktop.github.com/ +[link_git_rewriting]: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History diff --git a/dev_requirements.txt b/dev_requirements.txt index 608d2780b..4e0e0881c 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -8,3 +8,4 @@ numpydoc pytest pytest-cov requests +sphinx diff --git a/docs/developing.rst b/docs/developing.rst new file mode 100644 index 000000000..32900cbb4 --- /dev/null +++ b/docs/developing.rst @@ -0,0 +1,226 @@ +==================== +Developer Guidelines +==================== + +This webpage is intended to guide users through making making changes to +``tedana``'s codebase, in particular working with tests. +The worked example also offers some guidelines on approaching testing when +adding new functions. +Please check out our `contributing guide`_ for getting started. + + +Monthly Developer Calls +======================= +We run monthly developer calls via Zoom. +You can see the schedule via the tedana `google calendar`_. + +Everyone is welcome. +We look forward to meeting you there! + + +Adding and Modifying Tests +========================== +Testing is an important component of development. +For simplicity, we have migrated all tests to ``pytest``. +There are two basic kinds of tests: unit and integration tests. +Unit tests focus on testing individual functions, whereas integration tests focus on making sure +that the whole workflow runs correctly. + +Unit Tests +---------- +For unit tests, we try to keep tests from the same module grouped into one file. +Make sure the function you're testing is imported, then write your test. +Good tests will make sure that edge cases are accounted for as well as common cases. +You may also use ``pytest.raises`` to ensure that errors are thrown for invalid inputs to a +function. + +Integration Tests +----------------- +Adding integration tests is relatively rare. +An integration test will be a complete multi-echo dataset called with some set of options to ensure +end-to-end pipeline functionality. +These tests are relatively computationally expensive but aid us in making sure the pipeline is +stable during large sets of changes. +If you believe you have a dataset that will test ``tedana`` more completely, please open an issue +before attempting to add an integration test. +After securing the appropriate permission from the dataset owner to share it with ``tedana``, you +can use the following procedure: + +(1) Make a ``tar.gz`` file which will unzip to be only the files you'd like to +run a workflow on. +You can do this with the following, which would make an archive ``my_data.tar.gz``: + +.. code-block:: bash + + tar czf my_data.tar.gz my_data/*.nii.gz + +(2) Run the workflow with a known-working version, and put the outputs into a text file inside +``$TEDANADIR/tedana/tests/data/``, where ``TEDANADIR`` is your local ``tedana repository``. +We encourage using the convention ``__echo_outputs.txt``, appending ``verbose`` +to the filename if the integration test uses ``tedana`` in the verbose mode. + +(3) Write a test function in ``test_integration.py``. +To write the test function you can follow the model of our `five echo set`_, which takes the following steps: + +1. Check if a pytest user is skipping integration, skip if so +#. Use ``download_test_data`` to retrieve the test data from OSF +#. Run a workflow +#. Use ``resources_filename`` and ``check_integration_outputs`` to compare your expected output to + actual output. + +(4) If you need to upload new data, you will need to contact the maintainers and ask them to either add +it to `OSF`_ or give you permission to add it. + +(5) Once you've tested your integration test locally and it is working, you will need to add it to the +CircleCI config and the ``Makefile``. +Following the model of the three-echo and five-echo sets, define a name for your integration test +and on an indented line below put + +.. code-block:: bash + + @py.test --cov-append --cov-report term-missing --cov=tedana -k TEST + +with ``TEST`` your test function's name. +This call basically adds code coverage reports to account for the new test, and runs the actual +test in addition. + +(6) Using the five-echo set as a template, you should then edit ``.circlec/config.yml`` to add your +test, calling the same name you define in the ``Makefile``. + +Viewing CircleCI Outputs +------------------------ +If you need to take a look at a failed test on CircleCI rather than locally, you can use the +following block to retrieve artifacts (see CircleCI documentation here_) + +.. code-block:: bash + + export CIRCLE_TOKEN=':your_token' + + curl https://circleci.com/api/v1.1/project/:vcs-type/:username/:project/$build_number/artifacts?circle-token=$CIRCLE_TOKEN \ + | grep -o 'https://[^"]*' \ + | sed -e "s/$/?circle-token=$CIRCLE_TOKEN/" \ + | wget -v -i - + +To get a CircleCI token, follow the instructions for `getting one`_. +You cannot do this unless you are part of the ME-ICA/tedana organization. +If you don't want all of the artifacts, you can go to the test details and use the browser to +manually select the files you would like. + +Worked Example +============== +Suppose we want to add a function in ``tedana`` that creates a file called ```hello_world.txt`` to +be stored along the outputs of the ``tedana`` workflow. + +First, we merge the repository's ``master`` branch into our own to make sure we're up to date, and +then we make a new branch called something like ``feature/say_hello``. +Any changes we make will stay on this branch. +We make the new function and call it ``say_hello`` and locate this function inside of ``io.py``. +We'll also need to make a unit test. +(Some developers actually make the unit test before the new function; this is a great way to make +sure you don't forget to create it!) +Since the function lives in ``io.py``, its unit test should go into ``test_io.py``. +The job of this test is exclusively to tell if the function we wrote does what it claims to do +without errors. +So, we define a new function in ``test_io.py`` that looks something like this: + +.. code-block:: python + + def test_say_hello(): + # run the function + say_hello() + # test the function + assert op.exists('hello_world.txt') + # clean up + os.remove('hello_world.txt') + +We should see that our unit test is successful via + +.. code-block:: bash + + pytest $TEDANADIR/tedana/tests/test_io.py -k test_say_hello + +If not, we should continue editing the function until it passes our test. +Let's suppose that suddenly, you realize that what would be even more useful is a function that +takes an argument, ``place``, so that the output filename is actually ``hello_PLACE``, with +``PLACE`` the value passed and ``'world'`` as the default value. +We merge any changes from the upstream master branch into our branch via + +.. code-block:: bash + + git checkout feature/say_hello + git fetch upstream master + git merge upstream/master + +and then begin work on our test. +We need to our unit test to be more complete, so we update it to look more like the following, +adding several cases to make sure our function is robust to the name supplied: + +.. code-block:: python + + def test_say_hello(): + # prefix of all files to be checked + prefix = 'hello_' + # suffix of all files to be checked + suffix = '.txt' + # run the function with several cases + for x in ['world', 'solar system', 'galaxy', 'universe']: + # current test name + outname = prefix + x + suffix + # call the function + say_hello(x) + # test the function + assert op.exists(outname) + # clean up from this call + os.remove(outname) + +Once that test is passing, we may need to adjust the integration test. +Our program creates a file, ``hello_world.txt``, which the older version would not have produced. +Therefore, we need to add the file to ``$TEDANADIR/tedana/tests/data/tedana_outputs.txt`` and its +counterpart, R2-D2-- uh, we mean, ``tedana_outputs_verbose.txt``. +With that edit complete, we can run the full ``pytest`` suite via + +.. code-block:: bash + + pytest $TEDANADIR/tedana/tests + +Once that filename is added, all of the tests should be passing and we should open a PR to have our +change reviewed. + +From here, others working on the project may request changes and we'll have to make sure that our +tests are kept up to date with any changes made as we did before updating the unit test. +For example, if a new parameter is added, ``greeting``, with a default of ``hello``, we'll need to +adjust the unit test. +However, since this doesn't change the typical workflow of ``tedana``, there's no need to change +the integration test; we're still matching the original filename. +Once we are happy with the changes and some members of ``tedana`` have approved the changes, our +changes will be merged! + +We should then do the following cleanup with our git repository: + +.. code-block:: bash + + git checkout master + git fetch upstream master + git merge upstream/master + git branch -d feature/say_hello + git push --delete origin feature/say_hello + +and we're good to go! + + +.. _git: https://git-scm.com/ +.. _`git pro`: https://git-scm.com/book/en/v2 +.. _repository: https://github.com/ME-ICA/tedana +.. _Fork: https://help.github.com/en/github/getting-started-with-github/fork-a-repo +.. _`pull request`: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request +.. _GitKraken: https://www.gitkraken.com/ +.. _`GitHub Desktop`: https://desktop.github.com/ +.. _SourceTree: https://www.sourcetreeapp.com/ +.. _`GitHub UI`: https://help.github.com/en/github/managing-files-in-a-repository/editing-files-in-your-repository +.. _this: https://github.com/ME-ICA/tedana/tree/master/docs +.. _ReStructuredText: http://docutils.sourceforge.net/rst.html#user-documentation +.. _`five echo set`: https://github.com/ME-ICA/tedana/blob/37368f802f77b4327fc8d3f788296ca0f01074fd/tedana/tests/test_integration.py#L71-L95 +.. _here: https://circleci.com/docs/2.0/artifacts/#downloading-all-artifacts-for-a-build-on-circleci +.. _`getting one`: https://circleci.com/docs/2.0/managing-api-tokens/?gclid=CjwKCAiAqqTuBRBAEiwA7B66heDkdw6l68GAYAHtR2xS1xvDNNUzy7l1fmtwQWvVN0OIa97QL8yfhhoCejoQAvD_BwE#creating-a-personal-api-token +.. _`google calendar`: https://calendar.google.com/calendar/embed?src=pl6vb4t9fck3k6mdo2mok53iss%40group.calendar.google.com +.. _`contributing guide`: https://github.com/ME-ICA/tedana/blob/master/CONTRIBUTING.md diff --git a/docs/index.rst b/docs/index.rst index 7f02eb45e..2cf2bc8cd 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -152,6 +152,7 @@ tedana is licensed under GNU Lesser General Public License version 2.1. faq support contributing + developing roadmap api