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

Add test workflow #249

Closed
wants to merge 5 commits into from
Closed

Add test workflow #249

wants to merge 5 commits into from

Conversation

zharinov
Copy link
Contributor

Closes #248

@zharinov
Copy link
Contributor Author

zharinov commented Jul 28, 2022

Example: https://github.com/zharinov/zprint/runs/7554958077?check_suite_focus=true

UPD. A-a-and it stopped working once I've created this PR

@zharinov zharinov marked this pull request as draft July 28, 2022 07:22
@zharinov
Copy link
Contributor Author

zharinov commented Jul 28, 2022

Works with separated CLJ (ubuntu) and CLJS (macos).
I didn't manage to install Planck on ubuntu in reliable manner, so hopefully this separated approach would be a good start.

Example: https://github.com/zharinov/zprint/actions/runs/2752294939

UPD. ClojureScript testing is still quite slow, though

@zharinov zharinov marked this pull request as ready for review July 28, 2022 08:12
@kkinnear
Copy link
Owner

Wow, I'm impressed. I looked over the results from the runs you did. I haven't even tried Java 17, and you seem to have used it with no problems. That's cool on its own. You also did the clojure tests with the test-runner, which I've never done either. Overall, once you split the tests, it all looks like it worked pretty well. I've not tried any of this on Ubuntu, but I have no reason to think it wouldn't work, and you seem to have made it happen.

I expect your response to my comment in the Issue will elicit some additional information. I guess my question here is: Is this what you were intending to offer, and if I accept this PR, then you are finished? Or are you looking for a additional things that would be helpful, or what?

Thank you so much for doing this. I have already learned a lot. I knew nothing about Temurin Java, for instance. Thanks again!

I'm not going to accept this PR until we have additional dialog about what your plans are, but I expect that I will do so eventually.

@zharinov
Copy link
Contributor Author

Is this what you were intending to offer, and if I accept this PR, then you are finished? Or are you looking for a additional things that would be helpful, or what?

My intention was to make the first step, because at the moment I don't know how complete solution would look like. The further course of action can be something like this:

  1. Automate your current release workflow, so you don't have to perform it manually
  2. With feedback from newly created CI pipeline, we can refactor more confidently some tests and the workflow itself
  3. Once I became familiar with code base, I would like to contribute some new features to zprint itself 🙂

@zharinov
Copy link
Contributor Author

zharinov commented Jul 29, 2022

I knew nothing about Temurin Java, for instance.

I don't know much about current Java implementations either, but what I know is that temurin is kind of default, so GitHub Actions platform caches it while other distributions are being downloaded every run.

@kkinnear
Copy link
Owner

That sounds great.

I'm thinking that step one of getting some CI testing is almost there. I think that consists of:

  1. Getting some kind of Linux system working
  2. Getting Clojure working on it
  3. Running the zprint tests
  4. Running test_config with the uberjar
  5. Building zprintl-n.m.p
  6. Running test_config with zprintl-...

This is just my idea, I'm open to alternatives.

You've achieved 1-3. I'm thinking that #4 probably isn't too hard. Once you've build the uberjar, (which I think you have), you just:

./test_config 1.2.3 uberjar

Now, presently, test_config doesn't exit with a non-zero exit status if it fails. It just spits up vast quantities of junk which makes it pretty clear it failed, but that isn't going to be very useful in an automated CI environment. So I will have to figure out how to hack it to fail in a way that affects the exit status.

Doing #5 is ... interesting. I can show you how I do #5 "by hand", so to speak. I don't know how hard it will be to get that into a GitHub action. The short story on building zprintl... is that I use docker on an old Intel MacBook Air to do it. I haven't done it in several months (and so this might not work at all today). I don't have access to that laptop at the moment, so I can't verify that this works now. This might even work on my current M1 MacBook Air, but when last I tried it (which was maybe late 2020, early 2021) the linux docker image did work, but running graalVM to build zprint never terminated. It didn't fail with an error, it just never terminated. I gave it maybe 30 minutes, but nothing happened.

When building zprintl... on the old Intel laptop using docker, it takes 5 or 6 minutes to build. Never more than 10.

Anyway, the files are:

build.zprintl
zprintl.sh

This is the output on the screen when it builds:

Kims-2012-Air:zprint kkinnear$ ./build.zprintl java8-20.3.0 1.2.3
Downloading: Component catalog from www.graalvm.org
Processing Component: Native Image
Downloading: Component native-image: Native Image  from github.com
Installing new component: Native Image (org.graalvm.native-image, version 20.3.0
)
Refreshed alternative links in /usr/bin/
GraalVM Version 20.3.0 (Java Version 1.8.0_272-b10)
/app
[zprintl-1.2.3:106]    classlist:  12,501.82 ms,  5.82 GB
[zprintl-1.2.3:106]        (cap):   2,223.36 ms,  5.82 GB
[zprintl-1.2.3:106]        setup:   6,703.84 ms,  5.82 GB
[zprintl-1.2.3:106]     (clinit):   1,281.71 ms,  6.68 GB
[zprintl-1.2.3:106]   (typeflow):  76,022.62 ms,  6.68 GB
[zprintl-1.2.3:106]    (objects):  55,351.94 ms,  6.68 GB
[zprintl-1.2.3:106]   (features):   1,996.69 ms,  6.68 GB
[zprintl-1.2.3:106]     analysis: 138,804.21 ms,  6.68 GB
[zprintl-1.2.3:106]     universe:   4,398.94 ms,  6.68 GB
[zprintl-1.2.3:106]      (parse):  21,789.59 ms,  6.66 GB
[zprintl-1.2.3:106]     (inline):  16,512.05 ms,  6.41 GB
[zprintl-1.2.3:106]    (compile): 128,600.41 ms,  6.67 GB
[zprintl-1.2.3:106]      compile: 171,623.67 ms,  6.67 GB
[zprintl-1.2.3:106]        image:  18,345.02 ms,  6.59 GB
[zprintl-1.2.3:106]        write:  11,946.19 ms,  6.59 GB
[zprintl-1.2.3:106]      [total]: 365,157.80 ms,  6.59 GB

This is using an old version of graalvm, but whenever I upgrade something else breaks, and since this works fine, I haven't had any motivation for upgrading. I don't know if this version even exists anymore. I know that if you try to download graalvm, they don't even have a Jave8 version. Oh, that may be a problem. What I've shown here is how to build a Java8 version of zprintl. I think you will have a java17 uberjar, and so that probably won't work great with building a java8 version of zprintl. But maybe it doesn't matter.

I'm hoping that you can figure out how to get a working version of zprintl using GitHub actions. From that, you can then do the ./test_config 1.2.3 graalvm-linux thing and have it run the additional tests.

I'm also hoping that whatever you build in the GitHub action can be saved somewhere so that it can be used in the release, and I don't have to keep doing it by hand. I don't mind doing the MacOS one by hand, as I do it just for testing, but the linux one has been a pain since I upgraded my laptop in early 2020. I don't know how or where you can save results/output from a GitHub action, but that would be a goal. I'd just as soon not put it directly into a release, because it might be the release version. There is a whole thing about how to package up the release that we need to discuss, but that is probably not today's problem. I think today's problem is to have something to package up for a release, not so much how to do it.

Even without figuring out how to save a zprintl for a release, if you can get 1-5 working in your fork, I'll be delighted to take it.

@zharinov
Copy link
Contributor Author

Now, presently, test_config doesn't exit with a non-zero exit status if it fails. It just spits up vast quantities of junk which makes it pretty clear it failed, but that isn't going to be very useful in an automated CI environment. So I will have to figure out how to hack it to fail in a way that affects the exit status.

If successful test outputs nothing, it may be as easy as:

- name: Test config
  run: test "$(./test_config 1.2.3 uberjar)" = ""

I'll read your comment more carefully tomorrow, it's night time in my location.

@kkinnear
Copy link
Owner

Good point about test_config and output. However this is what a successful run looks like:

√ projects/zprint % ./test_config 1.2.4 graalvm-mac
If you ^c out of this test, be sure and clean up ~/.zprintrc, ./.zprintrc and ./.zprint.edn!
...........  --url
...........  --url, with URL missing, see if cache works
...........  --url, with URL missing, see if expired cache works
...........  --url, with URL missing, and no cache
...........  --url, with URL not a valid Clojure map, and no cache
...........  --url, with URL not a valid zprint options map, and no cache
...........  --url-only, with valid URL and no cache
...........  --url-only, with valid URL, file w/fn, and no cache
...........  basic config test.
...........  basic config with comment in ~/.zprintrc test.
...........  {:cwd-zprintrc? true} on command line test
...........  {:search-config? true} on command line test
...........  ~/.zprintrc being used test
...........  .zprintrc being used test
...........  -d test -- ~/.zprintrc should not be used
...........  bad ~/.zprintrc, invalid Clojure map test
...........  invalid zprint options map in ~/.zprintrc
...........  -v test
...........  -h test
...........  --explain-all test
...........  --explain test
...........  :coerce-to-false test
...........  fn using (fn ...) in ~/.zprintrc file test
...........  fn using #(...) in ~/.zprintrc file test
...........  fn #(...) in .zprintrc, unused, no :cwd-zprintrc?/:search-config?
...........  fn #(...) in .zprintrc found since :cwd-zprintrc?
...........  local .zprintrc file that does not parse found with :search-config?
...........  local .zprintrc file that does not parse found with :cwd-zprintrc?
...........  local .zprintrc file with bad data found with :search-config?
...........  local .zprintrc file with bad data found with :cwd-zprintrc?
...........  fn is valid in ./.zprintrc file test
...........  fn using (fn [x y] ...) on command line test
...........  fn using #(...) on command line test
...........  -w test with one file
...........  -w test with two files
...........  -w test with three files, one of which doesn't exist
...........  -w test with two files, the first which is read-only
...........  -w test with two files, the first which fails to format: bad !zprint map
...........  -w test with two files, the first which fails to format: bad !zprint key
...........  local .zprintrc having a fn that is used
...........  command line access to guide option-fn
...........  html output

Which is not useless information, but makes the "no output, it must have worked" not be a useful approach.

I'll work on test_config to get it to do exit-status stuff. Which I assume will allow it to be noticed if it doesn't work in a GitHub action?

Thanks!

@zharinov
Copy link
Contributor Author

Okay, I see now.

Another approach would be checking whether every non-first line of script output is starting from ........... or not. This one is more quick-and-dirty, but may be okay as the first phase of creating test pipeline. Up to you, though.

@kkinnear
Copy link
Owner

I just hacked test_config to exit with the number of tests that failed. I tested it by forcing a couple failures. I didn't try forcing every failure. I am in the end-stage of releasing 1.2.4, and after I build the zprintl with my old laptop Monday, I'll be releasing 1.2.4 on Tuesday so it will show up in the repo then. But I'll attach it to this comment now, in case it is helpful to. you in the meantime. I had to rename it to test_config.txt to get GitHub to let me attach it here. You will want to remove the .txt extension if you use it.
test_config.txt
.

Copy link
Contributor

@vemv vemv left a comment

Choose a reason for hiding this comment

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

A few humble suggestions

@@ -0,0 +1,82 @@
on:
- push
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that w/ this syntax it won't run on pull requests

uses: actions/setup-java@v3.4.0
with:
distribution: temurin
java-version: 17
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered using a matrix syntax? Running it on all the LTS JDK versions would seem reasonable.

Comment on lines +61 to +60
cli: latest
lein: latest
bb: latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered using fixed versions? It's often quite annoying to have CI builds break for seemingly no reason

Copy link
Contributor Author

@zharinov zharinov Aug 4, 2022

Choose a reason for hiding this comment

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

Yes, and actually I would like to propose using dependency updating the tool I'm actively contributing to: https://github.com/renovatebot/renovate

The app is free for GitHub, and it handles project.clj as well as deps.edn.

Using it, we would have both of the two worlds: fresh dependencies (thanks to Renovate) and merge confidence (thanks to CI tests)

Copy link
Contributor Author

@zharinov zharinov Aug 4, 2022

Choose a reason for hiding this comment

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

Here is my demo of what is possible with automatic dependency updates:
https://github.com/zharinov/zprint/pulls

@kkinnear What do you think?

(I need to fix Leiningen commented deps handling, though) — done

Copy link
Owner

Choose a reason for hiding this comment

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

Wow, that's amazing. As you may have noticed, I rarely change the versions of the things on which zprint depends, as I find it a pain to debug things like that when they break. Not that they break all that often, other than graalVM, which I noticed isn't something that renovate found. The thought of having major dependencies automatically updated (or at least automatically PR'ed for update) is kind of scary, but probably a good idea. As long as we really believe that the automated tests will run.

I didn't read too closely, but is it the case that if it automatically accepts the renovate PR's, and the tests don't run correctly, then it will back them out or something? Or is that a manual operation?

In any case, I'm up for giving renovate a try, but I think that first we need to get the tests to work in GitHub actions, including test_config, and at least the Linux pre-built binary (zprintl-1.2.n). It would be nice to do the Mac version as well, but I expect that is not possible with GitHub actions. Anyway, once the tests work, then adding renovate to the mix would seem like a good, if kind of unsettling, idea. Thanks for proposing it!

Copy link
Contributor Author

@zharinov zharinov Aug 5, 2022

Choose a reason for hiding this comment

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

It can create PRs for you to review and merge, but also can be configured to automatically merge non-major releases once all CI checks are passing, leaving major updates up to you. Also it can create Dependency Dashboard issue for managing dependencies from the single central place.

Actually, it's very configurable. For example, I think it's possible to make it upgrade clojure dependencies that are documented in the markdown files (using regex).

So again, recommend you to give it a try once we're done here with build/test pipeline.

Copy link
Contributor Author

@zharinov zharinov Aug 5, 2022

Choose a reason for hiding this comment

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

Speaking of GraalVM, it's also possible to upgrade it automatically (and test on CI), as soon as we refer to https://github.com/graalvm/graalvm-ce-builds releases somewhere in the code

run: clojure -M:cljtest:humane:runner

- name: Integration tests
run: ./test_config 1.2.3 uberjar
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a good idea to run test_config through Shellcheck.

Also for any .sh script I recommend the following header:

#!/usr/bin/env bash
set -Eeuxo pipefail
cd "${BASH_SOURCE%/*}/.."

- name: Integration tests
run: ./test_config 1.2.3 uberjar

test-cljs:
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to at least draft a lint job. https://github.com/clj-kondo/lein-clj-kondo needs particularly little setup (no graalvm installation, etc)

@zharinov
Copy link
Contributor Author

zharinov commented Aug 6, 2022

Status update: I managed to create workflow that is covering all six requirements mentioned earlier (see example).

However, it has some ugly parts (e.g., version string detection) and next week I'm about to refactoring initial solution.

@kkinnear
Copy link
Owner

kkinnear commented Aug 7, 2022

That's beyond wonderful! Thanks for the example, it was great to see it all working! I'm truly impressed.

Two questions:

  1. Where does the uploaded zprintl-1.2.4 end up? I saw that it was uploaded, but it wasn't clear on first glance where it landed.
  2. It looks like this works on every push to the repo? Since I am using one branch to collaborate with someone testing some of the changes I am making, I'm thinking that it would be good to only run the workflow when a push to master is done. (Yes, I know I should change it to main.). The theory then would be that I'd push to master when I was about to do a release, and if all of the tests worked, then I'd tag it as a release and push the tags and then gather the files into the release.

I'm seeing that actions will do MacOS as well as linux, so downstream it might be valuable to have all of the released binaries built by actions, instead of the MacOS one on my laptop. Then maybe there would be a separate action/workflow to build the release when I tagged and pushed the tags to GitHub. But that isn't today's task.

I'm very impressed and excited about where this is all going. I'm looking forward to having this all work. It is going to make releasing zprint so much easier! Thank you so much for pursuing this work!

@zharinov
Copy link
Contributor Author

zharinov commented Aug 7, 2022

  1. If I understand it correctly, it is uploading to the artifact storage internal to the GitHub actions. This is storage to share common data between jobs. What we want at the end is to put it to some downloadable space, so that it will be available in the release page. I don't know much about it yet.
  2. In further work, I'll separate this whole workflow into the smaller chunks depending on the feedback speed they're providing. For example, Babashka tests are ultra-fast, so we can require them to pass in the first place PR-level checks are also a good place for clj-kondo, etc. Once changes are in trunk, ideally it should be in the releasable state, so that you can quickly release changes. This can be build for the whole lineage of platforms, or maybe just linux binary build+test (via ./test_config), while macos and windows versions are waiting for release.

Today I've created new GitHub action for Clojure tooling that combines anther 3 actions (setup-java, setup-graalvm and setup-clojure) and makes them start a way faster thanks to caching. Unlike original actions, this one won't download Java distribution such as GraalVM (+native-image component) every run and you won't need to wait one extra minute just for starting something useful. I want to use it here.

@zharinov
Copy link
Contributor Author

zharinov commented Aug 7, 2022

One problematic thing I see now is that build automation for M1 processor. GitHub doesn't provide runners on this chip, so we need to build it somewhere else. I tend to think it should be CirrusCI, which is the place where the Babashka M1 binary is being built.

@kkinnear
Copy link
Owner

kkinnear commented Aug 8, 2022

So, I'm not currently providing M1 pre-built binaries. I have found the MacOS binaries built for Intel by graalVM to be quite fine when running on an M1 MacBook Air. I suppose that M1 specific binaries might be even better, but so far that hasn't made it near to the top of my list for things to do for zprint.

@zharinov
Copy link
Contributor Author

Seems like this one can be closed now

@zharinov zharinov closed this Oct 25, 2022
@zharinov zharinov deleted the add-github-actions branch October 25, 2022 13:37
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.

Setup CI workflow
3 participants