-
Notifications
You must be signed in to change notification settings - Fork 14
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
Setup Eldev, Makefile, Tests and Update Github Actions #35
Conversation
p4v4n
commented
Feb 13, 2024
•
edited
Loading
edited
- Setup Eldev and Makefile.
- Add 1 test with buttercup.
- Add github actions for tests and linting.
- Fix indentation and check-declare errors from Eldev.
You should also update the CI github action to use the Makefile. |
While you're at this you can also add a |
I have setup 1 unit test with buttercup and added running tests as part of the existing job in CI. Let me know if I update the job name or setup a separate job for tests. |
@@ -29,3 +29,6 @@ jobs: | |||
|
|||
- name: Lint the project | |||
run: eldev -dtT -C compile --warnings-as-errors | |||
|
|||
- name: Run tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep the tests here we should probably rename the workflow to something like "CI".
I was just writing a comment about this. :D For me it's a bit better to have separate actions, as it makes it easier to see what ran/passed/failed in the output. |
Cool. Will setup a new job for tests. I have a couple of more related questions.
Edit: I think I understand what github actions are compared to workflow/jobs now. So tests setup should be in the same file. The 2nd question now is just if I should update the filename as well. |
Yeah, I think it should just call
Separate job in the same workflow would be better. |
I separated compile/lint/test into different jobs. Current Status: lint is failing as |
I'm guessing @doublep might have some idea what's going wrong with |
The compat issue was fixed after switching to emacs29.1 instead of snapshot. |
Yeah, that sounds like plausible explanation. I had missed this announcement. I guess now all that's left is to fixed the lint errors and squash all the related commits together. |
- Use emacs29.1 instead of snapshot for lint to fix compat installation issue emacs-compat/compat@c98e141
- to fix "treesit.c file not found" error from eldev
Fixed the lint errors and cleaned up the commit history. |
Great work! Thanks! |
It appears to be an issue in To reproduce locally:
inside some project. You can see from the stacktrace that it happens inside |