Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use GitHub actions #1617

Merged
merged 8 commits into from
Nov 28, 2023
Merged

Use GitHub actions #1617

merged 8 commits into from
Nov 28, 2023

Conversation

lucc
Copy link
Collaborator

@lucc lucc commented Apr 15, 2023

This is to fix #1616

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

@lucc lucc requested a review from pazz April 15, 2023 09:59
@lucc lucc self-assigned this Apr 15, 2023
@lucc
Copy link
Collaborator Author

lucc commented Apr 15, 2023

@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 lucc force-pushed the github-actions branch 3 times, most recently from d2e1d96 to f699ad9 Compare June 6, 2023 19:52
@lucc
Copy link
Collaborator Author

lucc commented Jun 6, 2023

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
Copy link
Collaborator Author

lucc commented Jun 6, 2023

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

@pazz
Copy link
Owner

pazz commented Jun 14, 2023

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

@lucc lucc force-pushed the github-actions branch 4 times, most recently from d2618f3 to 959341b Compare June 14, 2023 20:42
@lucc
Copy link
Collaborator Author

lucc commented Jun 14, 2023

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
Copy link
Owner

pazz commented Jun 14, 2023

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
Copy link
Collaborator Author

lucc commented Jun 17, 2023

@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
Copy link
Collaborator Author

lucc commented Jun 17, 2023

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
Copy link
Owner

pazz commented Jun 18, 2023

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: 023cf16
Perhaps this broke tests but I did not notice due to Travis not working anymore..

@lucc
Copy link
Collaborator Author

lucc commented Jun 18, 2023

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

1345f9a 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 bd02a58.

For the hooks file I added 9b7f855 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 lucc mentioned this pull request Jul 28, 2023
@lucc
Copy link
Collaborator Author

lucc commented Aug 27, 2023

@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
Copy link
Owner

pazz commented Aug 28, 2023

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!

Python 3.7 is end of life since August:
https://peps.python.org/pep-0537/#lifespan

Python 3.12 is available in github actions.
@lucc lucc force-pushed the github-actions branch 2 times, most recently from 037b7b4 to 3af7c1d Compare November 27, 2023 23:35
Until we can fix the tests we just skip them in order to get a working
CI setup.
@lucc
Copy link
Collaborator Author

lucc commented Nov 28, 2023

@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
Copy link
Owner

pazz commented Nov 28, 2023

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

@pazz pazz merged commit e47953a into master Nov 28, 2023
16 checks passed
@delete-merged-branch delete-merged-branch bot deleted the github-actions branch November 28, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Github Actions
2 participants