Skip to content

Initial attempt to run on CircleCI #514

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

Merged
merged 4 commits into from
Feb 27, 2019
Merged

Initial attempt to run on CircleCI #514

merged 4 commits into from
Feb 27, 2019

Conversation

danielcompton
Copy link
Contributor

@danielcompton danielcompton commented Feb 22, 2019

Converts the CI config to use CircleCI. You can see an example of this running at https://circleci.com/gh/danielcompton/clojure-mode/tree/use-circle-ci.

A few notes:

  • CircleCI has a feature called orbs which can reduce duplication further, but I don't know enough about the commonalities to abstract things further at the moment.
  • I can add more Emacs versions to test against, but 25 latest, 26 latest, and master seemed like decent coverage. I don't have enough knowledge here really.
  • I tried running make test-checks in CI, but it didn't work. I'm not sure how/if that should be brought over?
  • CircleCI doesn't have the concept of allowable failures, I've included emacs master, but that will probably break on occasions.

Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our contribution guidelines.
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

Thanks!

@bbatsov
Copy link
Member

bbatsov commented Feb 22, 2019

Nice! You also have to install cask in the default steps, otherwise you can’t run the tests or the test checks.

@danielcompton
Copy link
Contributor Author

danielcompton commented Feb 22, 2019

Cask is already installed in those Docker images when you use the -dev variants https://github.com/Silex/docker-emacs. I think the tests are running correctly, although the output looks a little different than on Travis.

@bbatsov
Copy link
Member

bbatsov commented Feb 22, 2019

Oh, I didn’t know this. So, I guess we only really need to update the readme icon.

@bbatsov bbatsov requested a review from cichli February 23, 2019 11:30
Copy link
Member

@cichli cichli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this – everything looks great to me! Just two very minor nits in the inline comments.

CircleCI has a feature called orbs which can reduce duplication further, but I don't know enough about the commonalities to abstract things further at the moment.

It should become a bit more clear once we've migrated CIDER as well.

I tried running make test-checks in CI, but it didn't work. I'm not sure how/if that should be brought over?

Could you link to the logs?

@cichli
Copy link
Member

cichli commented Feb 25, 2019

@bbatsov could you enable Build forked pull requests here?

Also: I've been through and reviewed all the requested PRs, but I don't have the permissions to merge them.

@danielcompton
Copy link
Contributor Author

Thanks for the review, I think I've addressed all of the issues.

I tried running make test-checks in CI, but it didn't work. I'm not sure how/if that should be brought over?

Could you link to the logs?

https://circleci.com/gh/danielcompton/clojure-mode/12

@cichli
Copy link
Member

cichli commented Feb 26, 2019

Thanks! Looks like test-checks checks any .el file including the -autoloads.el and -pkg.el files if they're present. I think we could exclude them in that regex. On Travis we always ran this task in a fresh environment so it never saw those two files.

@bbatsov
Copy link
Member

bbatsov commented Feb 26, 2019

@cichli Now you’ve got the power.

@danielcompton
Copy link
Contributor Author

I've changed the ordering of test-checks to come before test-bytecomp. I tried to adjust the regular expression to exclude the generated files, but Emacs doesn't support lookahead regular expressions, so I don't think it's possible, at least not without writing another function to filter the list afterwards.

@cichli cichli merged commit 6d8e5a6 into clojure-emacs:master Feb 27, 2019
@cichli
Copy link
Member

cichli commented Feb 27, 2019

Thanks for this! 🙌

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.

3 participants