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

Setup Eldev, Makefile, Tests and Update Github Actions #35

Merged
merged 6 commits into from
Feb 13, 2024

Conversation

p4v4n
Copy link
Contributor

@p4v4n p4v4n commented Feb 13, 2024

  • Setup Eldev and Makefile.
  • Add 1 test with buttercup.
  • Add github actions for tests and linting.
  • Fix indentation and check-declare errors from Eldev.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

You should also update the CI github action to use the Makefile.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

While you're at this you can also add a test folder and one trivial buttercup test, so we'd have the unit test infrastructure in place. (as in clojure-mode)

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 13, 2024

While you're at this you can also add a test folder and one trivial buttercup test, so we'd have the unit test infrastructure in place. (as in clojure-mode)

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

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".

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

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.

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.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 13, 2024

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.

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.

  1. What is currently running under lint is actually eldev compile. Should this be updated as well?
  2. I am just getting familiar with github actions terminology.Should tests be a separate job inside this workflow/file or a separate workflow?
    Adding tests as a separate workflow might be costly(assuming each workflow runs on a different VM) once we setup tests on every platform. If we are adding it to the same workflow/file the filename should probably be changed.

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.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

What is currently running under lint is actually eldev compile. Should this be updated as well?

Yeah, I think it should just call make lint.

I am just getting familiar with github actions terminology.Should tests be a separate job inside this workflow/file or a separate workflow?

Separate job in the same workflow would be better.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 13, 2024

Separate job in the same workflow would be better.

I separated compile/lint/test into different jobs.

Current Status: lint is failing as compat package cannot be installed on CI. I have tried a few things but none of them fixed the issue so far.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

I'm guessing @doublep might have some idea what's going wrong with compat.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 13, 2024

I'm guessing @doublep might have some idea what's going wrong with compat.

The compat issue was fixed after switching to emacs29.1 instead of snapshot.
Likely related to the below news. compat apparently is already part of emacs30 and will not be installed again.
emacs-compat/compat@c98e141

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

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.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 13, 2024

I guess now all that's left is to fixed the lint errors and squash all the related commits together.

Fixed the lint errors and cleaned up the commit history.

@p4v4n p4v4n changed the title Setup Eldev and Makefile Setup Eldev, Makefile, Tests and Update Github Actions Feb 13, 2024
@bbatsov bbatsov merged commit 9bbae69 into clojure-emacs:main Feb 13, 2024
3 checks passed
@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2024

Great work! Thanks!

@p4v4n p4v4n deleted the setup-eldev branch February 14, 2024 11:28
@doublep
Copy link

doublep commented Feb 14, 2024

I'm guessing @doublep might have some idea what's going wrong with compat.

It appears to be an issue in package-lint, not Eldev. Seems to be fixed upstream in this commit, but as Eldev by default issues the latest stable version, you can bump into the bug if using Emacs 30.

To reproduce locally:

$ eldev clean .eldev; eldev docker master -d lint package

inside some project. You can see from the stacktrace that it happens inside package-lint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants