Feature install action

[JulianGoeltz]

Add new job to test.yml for testing installation, responding to #235. This takes care of the first part, testing the local installation.

The shell test is not included (yet), as there is a dependence on the local path. If I cd to the package directory to have this path available, it is not a test of the execution anymore IIUC. Any idea for a fix there, or should we just leave that test out?

Regarding the test after pushing to PyPi: my understanding is that the GitHub Actions are executed responding to changes in the repository. This publish.yml would have to be executed after a push to PyPi, do you know of a way to trigger this?

[torik42]

The shell test is not included (yet), as there is a dependence on the local path. If I cd to the package directory to have this path available, it is not a test of the execution anymore IIUC. Any idea for a fix there, or should we just leave that test out?

I think you can safely cd into the repo as pytest will not load the modules from that folder (unless invoked as python -m pytest …, see here).

Regarding the test after pushing to PyPi: my understanding is that the GitHub Actions are executed responding to changes in the repository. This publish.yml would have to be executed after a push to PyPi, do you know of a way to trigger this?

My plan is the following: Bump version number, add git tag, and push. The action then publishes to test.pypi, waits and installs from test.pypi (similar to your test here). If that works, I would manually make a release on GitHub which publishes to PyPI. But I can add that myself drawing inspiration from the gridspeccer workflow and test locally.

[torik42]

I think you can safely cd into the repo as pytest will not load the modules from that folder (unless invoked as python -m pytest …, see here).

With my other comment, you don’t actually need to cd. If we want to be extra save, we could remove the yalafi folder before running the tests. Then we could even run with python -m pytest … and thus run all tests, right?

[JulianGoeltz]

I am at a loss here, neither

          cd YaLafi
          rm -rf yalafi
          python -m pytest tests

(failing with No module named 'yalafi.packages')
nor

          cd YaLafi
          pytest tests

(failing with No module named 'tests')
works.
I'm sure this is a trivial error on my side, but can't find it.

[torik42]

(failing with No module named 'yalafi.packages') nor

          cd YaLafi
          pytest tests

(failing with No module named 'tests') works. I'm sure this is a trivial error on my side, but can't find it.

This is because some tests load parts of others with import tests.something which doesn’t work if the root directory is not in sys.path. The others could have a similar reason, but I don’t seem to have that problem locally.

[torik42]

If we really do all tests after a local install, then we could ditch the other job. But I think having pytest tests/test_shell/test_shell.py here is good enough.

[JulianGoeltz]

Yes, the other job would be unnecessary then, but I prefer it like this.
When only the shell test is executed it runs through as well :)

[torik42]

Very good. Have you seen my other two comments in the code? I think it doesn’t test the PR it is called by right now.

[JulianGoeltz]

Have you seen my other two comments in the code?

Sorry, no, I don't see other comments. Where can I find them?

I think it doesn’t test the PR it is called by right now.

You mean this PR #236? When I push to the branch, it tells me something along the lines "new workflow needs authorisation by a maintainer". As you (I guess) approved this, the workflows were tested, and for me it says "All checks have passed. 8 successful checks".

By the way, thanks for responding so quickly, this is a positive experience!

[torik42]

Sorry, first time using the review feature. Didn’t know I need to submit the comments again.

[torik42]

I am at a loss here, neither

          cd YaLafi
          rm -rf yalafi
          python -m pytest tests

(failing with No module named 'yalafi.packages')

Hopefully, this is fixed now with my last commit on master.

[JulianGoeltz]

Addressed your comments :)

[torik42]

Thank you for working it out, it looks very good.

If you are okay with that, I will squash all your commits into one and force push to this branch to reduce clutter. You will still be the author (just not the committer) of the squashed commit. But you shouldn’t continue working on top of your commits for now.

[JulianGoeltz]

Sounds good