Nosetest to Pytest and travis.yml to GitHub Actions #87

ElijahCapps · 2024-06-05T00:57:13Z

This is a draft pull request for review before further modification addressing some issues I mentioned in #86, specifically migrating from nosetest to pytest and migrating from travis.yml to GitHub Actions. I used the nose2pytest package to do most of the migration from nosetest to pytest, and I manually changed a few assertions that weren't automatically caught (like assert raises). I used a workflow template from GitHub for testing a Python script, currently set to run Python v3.10 on Ubuntu. This can be changed to also include Python 2.7 and run on more systems, but I want to get the rest of the changes looked at first. The tests passed and the workflow automatically runs tests with every push.

My work will be reviewed by Madicken Munk.

Used the module nose2pytest to move from nosetest to pytest and used a starter workflow from GitHub Actions to replace the travis.yml file.

Added workflow_dispatch to manually run the workflow if desired.

Removed nosetest references that threw errors in the new workflow ("no module found").

Removed old nosetest assert_raise assertions and replaced with pytest.raises.

Removed travis.yml (forgot to do this earlier), also updated on_push.yml to be more explicit with which branch to test on a push.

Removed old nosetest assertion_raises from more tests, should be all of them now.

Attempting to ignore a "undefined name: @with_setup" error from flake8

@pytest

Removed more old nosetest functions (like @with_setup), replaced with @pytest.fixture, removed earlier error suppression from on_push.yml.

Changed incorrect "assert error.value == ErrorType" to correct "assert error.type is ErrorType"

Fixed a type "n_precursor" to "n_precursors"

katyhuff

Some high level thoughts included.

katyhuff · 2024-06-13T13:37:57Z

.github/workflows/on_push.yml

+# This workflow will install Python dependencies, run tests and lint with a single version of Python
+# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python
+
+name: Python application


I suspect this should be "PyNE" rather than "Python application".

katyhuff · 2024-06-13T13:43:28Z

.github/workflows/on_push.yml

+    - name: Set up Python 3.10
+      uses: actions/setup-python@v3
+      with:
+        python-version: "3.10"


Historically, we've tried to tests the build on multiple python versions (including python 2.7 to ensure backwards compatibility with python 2.). This is often called a build matrix.

It looks like GitHub has dropped support for running Python 2.7 in workflows:
actions/setup-python#672
Someone in the thread proposed a workaround that I could try implementing:
actions/setup-python#672 (comment)

@ElijahCapps I saw your comment on Slack that GitHub workflows don't support Python 2.7 builds. Let's discuss that here (not in the general channel on the ARFC slack).

What is the reasoning for moving from Travis to GitHub Actions anyway? (@munkm I'm sure there's a reason, I just haven't been in the loop for the last few years... Maybe you can explain why?)

I believe Munk told me to move it from Travis to Actions because Actions is newer.

It's not because actions is newer, it's because Travis removed their free runs and you now need to pay for CI jobs with them. Most projects had to migrate from Travis in 2020/2021 because of this issue.

Travis on most projects were given 10k credits for their project, and then it was pushed to a paid tier. This isn't super sustainable, though the Travis educational credits may be active again (from my dusty recollection for a while this wasn't an option either).

Oh, that makes a lot more sense.

@katyhuff I'm looking at using images from DockerHub to run Python versions 2.7 and 3.10 in their own containers, but I'm unsure of which versions to pick. There are a lot available and it lists many as having vulnerabilities I'm not familiar with. What would you suggest?

katyhuff · 2024-06-13T13:51:48Z

pyrk/db/tests/test_database.py

@@ -1,5 +1,3 @@
-from nose.tools import assert_equal, assert_true, assert_false
-
 from pyrk.db import database as d

 import unittest


It seems a bit strange that we aren't changing this to "import pytest" .

Oops, I missed that one!

Updated the workflow to have a build matrix for Python versions 2.7 and 3.10. Also migrated database test from unittest to pytest.

Fixed a syntax error

Fixed another syntax error

GitHub no longer supports hosting Python 2.7 through checkout. Switched to DockerHub to host different versions of Python.

Accidentally commented the checkout action

Using an older version of numpy (might be an incompatibility between pandas and newer versions at the moment).

Changed workflow to test Python 3.8, 3.9, 3.10, 3.11, 3.12. No longer testing Python 2.7. Earlier, fixed the numpy version to 1.26.4 in requirements.txt, as the latest (numpy 2.x) is currently incompatible with pandas.

Removed Python 3.8 from build matrix as it doesn't support later versions of numpy and will be losing support by October.

Pytest Conversion 2

katyhuff

Thanks! This is looking much better, but there are some formatting issues (please use your text editor to reindent for proper alignment.) and a few questions.

katyhuff · 2024-08-16T18:07:00Z

.github/workflows/on_push.yml

@@ -0,0 +1,46 @@
+# This workflow will install Python dependencies, run tests and lint with a single version of Python


This seems to go over the standard line character limit (usually 80 chars).
Also, my understanding is that this actually will test with multiple versions of python now (3.9, 10, 11, and 12). Perhaps correct this docstring so that it doesn't imply we're testing on just one version?

katyhuff · 2024-08-16T18:08:49Z

.github/workflows/on_push.yml

@@ -0,0 +1,46 @@
+# This workflow will install Python dependencies, run tests and lint with a single version of Python
+# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python


line too long.

katyhuff · 2024-08-16T18:10:05Z

.github/workflows/on_push.yml

+
+on:
+  push:
+    branches: [ "master", "main" ]


There is no "main" branch in this repository. I'm surprised this doesn't throw an error.
I'm perfectly happy for "master" to be renamed to "main" as is the norm now. However, there is no reason to have both.

katyhuff · 2024-08-16T18:22:16Z

.github/workflows/on_push.yml

+      run: |
+        # stop the build if there are Python syntax errors or undefined names
+        flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
+        # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide


We follow PEP8 in this project, for which the line limit is 80 chars, not 127.

I see this file came from here: https://github.com/actions/starter-workflows/blob/main/ci/python-app.yml . While GitHub may care about GitHub editor linelength, real python projects do not. PyRK should pass a clean flake8 run (as it used to (see line 44 of the old .travis.yml.))

katyhuff · 2024-08-16T18:22:51Z

.github/workflows/on_push.yml

+        flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
+    - name: Test with pytest
+      run: |
+        pytest


Are you sure this shouldn't be "pytest pyrk" ?

Also, it seems this workflow doesn't run coveralls (we used to run coveralls to get a notion of test coverage.). Is there a reason not to do that anymore?

I'm not familiar with coveralls, but I can look into setting it up in workflows as the next step in this pull request.

katyhuff · 2024-08-16T18:45:41Z

pyrk/tests/test_convective_model.py

                 2 * units.pascal * units.second)
    rho = 100 * units.kg / units.meter**3
-    assert_equal(h_wakao.h(rho, 0 * units.pascal * units.second),
+    assert (h_wakao.h(rho, 0 * units.pascal * units.second) ==
                 h_wakao.h(rho, 2 * units.pascal * units.second))


This needs to be re-indented.

katyhuff · 2024-08-16T18:45:48Z

pyrk/tests/test_density_model.py

+    assert dm_linear.model == 'linear'
+    assert dm_linear.rho(0 * units.kelvin) == alpha
+    assert dm_linear.rho() == alpha
+    assert (dm_linear.rho(1 * units.kelvin) ==
                 alpha + beta * 1.0 * units.kelvin)


This needs to be re-indented.

katyhuff · 2024-08-16T18:45:52Z

pyrk/tests/test_density_model.py

+    assert dm_flibe.model == 'linear'
+    assert dm_flibe.rho(0 * units.kelvin) == a_flibe
+    assert dm_flibe.rho() == a_flibe
+    assert (dm_flibe.rho(1 * units.kelvin) == a_flibe +
                 b_flibe * 1.0 * units.kelvin)


This needs to be re-indented.

katyhuff · 2024-08-16T18:46:31Z

pyrk/tests/test_th_system.py

-    assert_equal(th.conduction_slab(components[0], components[1], 0,
-                                    1 * units.meter, 1 * units.meter**2), 0)
+    assert th.conduction_slab(components[0], components[1], 0,
+                                    1 * units.meter, 1 * units.meter**2) == 0


This needs to be re-indented.

katyhuff · 2024-08-16T18:48:40Z

requirements.txt

@@ -1,5 +1,5 @@
 matplotlib
-numpy
+numpy==1.26.4


Why pin numpy to this version number? Are numpys later than this non-functional with pyrk? If so, I recommend numpy<=1.26.4. If it needs to be at least that version fo numpy (and later versions are ok, but earlier ones aren't) then we would use the opposite (numpy>=1.26.4).

I pinned it to 1.26.4 because errors kept popping up for a potential "binary incompatibility", which last I researched had to do with newer versions of numpy not working well with pandas. This currently seems to still be the case, at least for the test run in Python 3.9.

Fixed long comment lines, removed some unnecessary information, changed max line length to 80 characters and clarified pytest tests pyrk.

Went through and fixed all listed indent errors. Also changed requirements to accept the latest version of numpy again, as it seems its issue with pandas has been resolved.

Pinned numpy to 1.26.4 or earlier because apparently the issue with pandas is still present (in some versions of Python maybe?). Either way, it failed the test in v3.9.

ElijahCapps

Addressed all comments. Only thing that hasn't been implemented still is coveralls.

katyhuff

Great work, merging.

ElijahCapps added 10 commits May 31, 2024 15:42

Migrate nosetest to pytest and travis to GitHub Actions

cdcb06b

Used the module nose2pytest to move from nosetest to pytest and used a starter workflow from GitHub Actions to replace the travis.yml file.

Update on_push.yml

3bce78b

Added workflow_dispatch to manually run the workflow if desired.

Remove nosetest References

6513426

Removed nosetest references that threw errors in the new workflow ("no module found").

Update test_validation.py

626e0ea

Removed old nosetest assert_raise assertions and replaced with pytest.raises.

Removed travis.yml and Updated on_push.yml

f1edad1

Removed travis.yml (forgot to do this earlier), also updated on_push.yml to be more explicit with which branch to test on a push.

Removed more nosetest assertions

1537d15

Removed old nosetest assertion_raises from more tests, should be all of them now.

Update on_push.yml

949817d

Attempting to ignore a "undefined name: @with_setup" error from flake8

Rollback on_push.yml, remove more nostest

d13bb4e

Removed more old nosetest functions (like @with_setup), replaced with @pytest.fixture, removed earlier error suppression from on_push.yml.

Fixed incorrect pytest assertions

8c71949

Changed incorrect "assert error.value == ErrorType" to correct "assert error.type is ErrorType"

Update test_neutronics.py

0be49dc

Fixed a type "n_precursor" to "n_precursors"

katyhuff self-requested a review June 12, 2024 19:09

katyhuff reviewed Jun 25, 2024

View reviewed changes

ElijahCapps and others added 9 commits July 19, 2024 14:29

Updated workflow and database test

68611a7

Updated the workflow to have a build matrix for Python versions 2.7 and 3.10. Also migrated database test from unittest to pytest.

Update on_push.yml

57433ba

Fixed a syntax error

Update on_push.yml

9911e96

Fixed another syntax error

Update on_push.yml

70fffa4

GitHub no longer supports hosting Python 2.7 through checkout. Switched to DockerHub to host different versions of Python.

Update on_push.yml

0e6fc0f

Accidentally commented the checkout action

Update requirements.txt

b95875a

Using an older version of numpy (might be an incompatibility between pandas and newer versions at the moment).

Update on_push.yml

aee7023

Changed workflow to test Python 3.8, 3.9, 3.10, 3.11, 3.12. No longer testing Python 2.7. Earlier, fixed the numpy version to 1.26.4 in requirements.txt, as the latest (numpy 2.x) is currently incompatible with pandas.

Update on_push.yml

97e0145

Removed Python 3.8 from build matrix as it doesn't support later versions of numpy and will be losing support by October.

Merge pull request #1 from ElijahCapps/pytest-conversion-2

cd72348

Pytest Conversion 2

ElijahCapps marked this pull request as ready for review July 25, 2024 16:24

ElijahCapps requested a review from katyhuff August 12, 2024 14:14

katyhuff requested changes Aug 16, 2024

View reviewed changes

ElijahCapps added 3 commits September 5, 2024 17:54

Update on_push.yml

cef3b12

Fixed long comment lines, removed some unnecessary information, changed max line length to 80 characters and clarified pytest tests pyrk.

Fixing Indents

b91dbea

Went through and fixed all listed indent errors. Also changed requirements to accept the latest version of numpy again, as it seems its issue with pandas has been resolved.

Update requirements.txt

a0972ee

Pinned numpy to 1.26.4 or earlier because apparently the issue with pandas is still present (in some versions of Python maybe?). Either way, it failed the test in v3.9.

ElijahCapps commented Sep 5, 2024

View reviewed changes

ElijahCapps requested a review from katyhuff September 5, 2024 23:27

ElijahCapps assigned katyhuff Sep 24, 2024

katyhuff approved these changes Sep 24, 2024

View reviewed changes

katyhuff merged commit 37f2841 into pyrk:master Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nosetest to Pytest and travis.yml to GitHub Actions #87

Nosetest to Pytest and travis.yml to GitHub Actions #87

ElijahCapps commented Jun 5, 2024

katyhuff left a comment

katyhuff Jun 13, 2024

katyhuff Jun 13, 2024

ElijahCapps Jul 19, 2024

katyhuff Jul 19, 2024

ElijahCapps Jul 19, 2024

munkm Jul 19, 2024 •

edited

Loading

ElijahCapps Jul 19, 2024

ElijahCapps Jul 22, 2024

katyhuff Jun 13, 2024

ElijahCapps Jun 25, 2024

katyhuff left a comment

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

ElijahCapps Sep 5, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

katyhuff Aug 16, 2024

ElijahCapps Sep 5, 2024

ElijahCapps left a comment

katyhuff left a comment

		@@ -0,0 +1,46 @@
		# This workflow will install Python dependencies, run tests and lint with a single version of Python

		@@ -0,0 +1,46 @@
		# This workflow will install Python dependencies, run tests and lint with a single version of Python
		# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python

Nosetest to Pytest and travis.yml to GitHub Actions #87

Nosetest to Pytest and travis.yml to GitHub Actions #87

Conversation

ElijahCapps commented Jun 5, 2024

katyhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munkm Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katyhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElijahCapps left a comment

Choose a reason for hiding this comment

katyhuff left a comment

Choose a reason for hiding this comment

munkm Jul 19, 2024 •

edited

Loading