pazz / alot

Terminal-based Mail User Agent
GNU General Public License v3.0
681 stars 162 forks source link

Use GitHub actions #1617

Closed lucc closed 7 months ago

lucc commented 1 year ago

This is to fix #1616

The workflow run can be seen here: https://github.com/pazz/alot/actions?query=branch%3Agithub-actions

lucc commented 1 year ago

@pazz I just noticed the trouble we had with the python notmuch bindings in CI back on travis. Are you still on the Notmuch mailing list? Can you urge them to publish newer versions of the notmuch2 python bindings to pipy?

The page is here:https://pypi.org/project/notmuch/ the maintainer seems to be @weilbith (maybe he will see this)

lucc commented 1 year ago

The workflow now runs succesful and shows that the tests currently fail.

@pazz I suggest we finish this PR first (refactor, rewrite git hist) and fix the tests later on, what do you think?

lucc commented 1 year ago

The merge status here on Github is still waiting for CI @ traivs. @pazz maybe you can remove that requirement from the repo settings?

pazz commented 1 year ago

I've removed the travis requirement now. Some of the new tests seem to fail though?

lucc commented 1 year ago

Sorry I fixed the test setup, I think the remaining tests are actual test failure. Two of them are also excluded in the nixpkgs package description, so I assume they have been broken for some time.

pazz commented 1 year ago

Just checking: https://github.com/pazz/alot/actions/runs/5271916691/jobs/9533462587?pr=1617#step:9:566 (line 565) this one actually looks like the notmuch environment is not set up properly?

lucc commented 1 year ago

@pazz do you mean this?:

ERROR: test_save_named_query (tests.db.test_manager.TestDBManager)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/alot/alot/alot/db/manager.py", line 92, in flush
    db = Database(path=self.path, mode=mode)
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/notmuch2/_database.py", line 160, in __init__
    raise errors.NotmuchError(ret, msg)
notmuch2.NoConfigError: Error: cannot load config file.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/alot/alot/tests/db/test_manager.py", line 49, in test_save_named_query
    self.manager.flush()
  File "/home/runner/work/alot/alot/alot/db/manager.py", line 175, in flush
    raise e
  File "/home/runner/work/alot/alot/alot/db/manager.py", line 94, in flush
    raise DatabaseLockedError()
alot.db.errors.DatabaseLockedError

I think this is not a problem with the notmuch setup but with the test itself as the first exception is the missing config file. No idea (yet) why the setup code does not work.

I can reproduce this test failure on a fresh ubuntu container using system python with a hand compiled notmuch and notmuch python bindings (similar to the github action) and this test is also excluded in nixpkgs. I assume that notmuch is installed correctly in nixpkgs because I use that distro and never had any problems with alot or notmuch.

lucc commented 1 year ago

I just added touch ~/.notmuch-config to the workflow and the original test failure is gone but we get a different one that checks what happens if no notmuch config file is found: https://github.com/pazz/alot/actions/runs/5297609415/jobs/9589493316#step:10:550

pazz commented 1 year ago

Yes, I was referring to a "cannot load config file" error before and your last commit seems to have affected this. Can you try adding a dummy hooks.py file? Lots of warnings in the most recent runs about that, in unrelated tests.

There has recently been some PR changing the way alot accesses the notmuch config: https://github.com/pazz/alot/commit/023cf168adffc5ac80e40f0e5e75f02a8c431750 Perhaps this broke tests but I did not notice due to Travis not working anymore..

lucc commented 1 year ago

@pazz which commit do you mean by "your last commit"?

1345f9a579c8ae6c711a4e9e691e0badad5e84f4 is not part of this PR. I put this commit in a different branch (test-config-file) deliberatly because I still belive that the worflow setup in this PR is correct and the remaining test failures are actually broken tests in our codebase.

The last commit in this PR is bd02a584a598e995830cbddc321b3f37dc328e2a.

For the hooks file I added 9b7f855bc00bb1ed51c23d7669164d21f821caa7 to the test-config-file branch: I can't seem to find the warnings you reffer to or check if they vanish with this.

In general I would like to not add global config files in the VM/container of the github actions workflow because that will only fix/hide the warnings in CI. I think it is much better to fix the source in alot itself or properly mock these things in the test suite. That way the fix would propagate to all different kind of places: our local dev env where we run the tests, CI, the downstream CI used by different packageing teams, etc.

lucc commented 10 months ago

@pazz can you please have another look at this. I would like to get this merge as soon as possible (also prompted by #1626).

If this ci pipeline is to convoluted we could also have a simple nix build which would catch catch errors like #1626 as well.

pazz commented 10 months ago

I don't think this looks too complicated, especially compared to what we had on travis. However, I've not had (and am unlikely to have soon) time to read up on GitHub actions and nix etc. So my review is not that deep. If you think this will improve things for yourself and others, by all means go ahead and merge this. Thanks!

lucc commented 7 months ago

@pazz I removed support for python 3.7 because this is EOL. Is it ok with you to merge this change together with the CI stuff?

I have now reached a point where CI runs but I had to ignore some tests in a hacky way. But I would prefer to have CI with a workaround rather than no CI at all. So I would now merge this.

pazz commented 7 months ago

Thats fine with me. I agree that partially broken/hacky tests are better than none. Really appreciate all your work on this!