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

Continuous integration: tests on GitHub runner #11

Merged
merged 27 commits into from
Jul 31, 2023

Conversation

slobentanzer
Copy link
Collaborator

Establishes a simple CI pipeline that runs all tests on a GitHub runner in case of a push to master or a PR.

Adds a badge for the status of the pipeline to the README.

slobentanzer and others added 12 commits July 26, 2023 16:23

Verified

This commit was signed with the committer’s verified signature.
miscco Michael Schellenberger Costa

Verified

This commit was signed with the committer’s verified signature.
miscco Michael Schellenberger Costa
@ktpolanski
Copy link
Contributor

ktpolanski commented Jul 31, 2023

How do I go look at the tests?

@slobentanzer
Copy link
Collaborator Author

slobentanzer commented Jul 31, 2023

The GitHub action specified in .github/workflows will start a VM on GitHub server (just linux for now) and execute the test suite on that VM. This happens each time someone pushes to master directly or in the event of a pull request.

If all tests pass, the process finishes successfully, and the badge will be green (tests passing). If something is amiss (at least one test fails), the process will also fail, informing you of a potential issue; the badge will be red.

The VM is temporary and disappears after the action is run.

@ktpolanski
Copy link
Contributor

I clicked the badge and it's just an image? Is there no log of how the testing went?

@slobentanzer
Copy link
Collaborator Author

You can see the log in the 'Actions' tab here on GitHub.

@slobentanzer
Copy link
Collaborator Author

Here's how that looks on my fork: https://github.com/slobentanzer/sctk/actions

@ktpolanski
Copy link
Contributor

How will this look in the event of a PR? Will there be an automated thing at the bottom with test results?

Is there some way to have the readme badge take the clicker to the set of tests the badge is in reference to?

@slobentanzer
Copy link
Collaborator Author

How will this look in the event of a PR? Will there be an automated thing at the bottom with test results?

You can run automated tests that are added to the PR; that's what I would suggest. They will appear in the review section as automated checks. I haven't done this yet on my ow repos, but have seen it a few times and it seems straightforward. E.g. in the scanpy repository.

Is there some way to have the readme badge take the clicker to the set of tests the badge is in reference to?

Not in the way this is implemented. The svg is generated by GitHub just as a render of the Action either passing or failing, not much room for customisation. I want to add more badges, and some services (e.g. codeclimate) offer more flexibility in the badges. We can open a discussion on what to add / use involving more people from your side.

@ktpolanski
Copy link
Contributor

Does the automated testing on PR need to be somehow set up from the repo, or will it work in the current configuration?

The Scanpy badge images are links. https://raw.githubusercontent.com/scverse/scanpy/master/README.md

@slobentanzer
Copy link
Collaborator Author

The yaml file in the workflows directory is the setup of the action. I think there are general settings to allow / prevent running actions, but the code itself should be enough to trigger the runner.

The Scanpy badge images are links.

I understand what you mean now. You can have the badge link wherever you want, but where would we link in case of the test suite? To the actions tab, or to the test directory on GitHub? We could also link to the testing section of the contributor guide (which we need to add).

@ktpolanski
Copy link
Contributor

So you're saying that in the current form, a PR should trigger the tests, and they should be displayed clearly within the PR?

Ideally it would direct right to the logs of the testing that the badge is referring to.

@slobentanzer
Copy link
Collaborator Author

So you're saying that in the current form, a PR should trigger the tests, and they should be displayed clearly within the PR?

The basic application is: upon PR or push to master, the pipeline runs and either succeeds or fails.

This can then be used for downstream applications:

  • badge in the README
  • automatic checking of PR via automated review

Ideally it would direct right to the logs of the testing that the badge is referring to.

There is a difference between where the badge directs (freely configurable via the markdown link), and how the automated review is displayed. See for example the most recent PR on scanpy for a number of automated checks and how they are displayed: scverse/scanpy#2575

@ktpolanski
Copy link
Contributor

Exactly, that scanpy PR is a good example of what I hope will show up when a PR is made here. I'm inferring from context that it should?

Ideally the badge would link to the latest test: https://github.com/slobentanzer/sctk/actions/runs/5714779629
That failing, the equivalent of this seems like a reasonable compromise: https://github.com/slobentanzer/sctk/actions/workflows/ci-cd.yaml

@slobentanzer
Copy link
Collaborator Author

slobentanzer commented Jul 31, 2023

Exactly, that scanpy PR is a good example of what I hope will show up when a PR is made here. I'm inferring from context that it should?

scanpy use an Azure runner (separate cloud-based solution) that allows more fine-tuning of the workflows. But in general, this is how it should look. I would go ahead with what is possible through GitHub actions for now, as it is much easier to implement.

Ideally the badge would link to the latest test

I think GitHub actions do not provide this information to automatically link to the latest test. Linking the actions page is an option for the badge on the README; in the case of automated PR checks (as in the scanpy PR, see e.g. the first one, "Pull Request Validation"), the "Details" link will automatically take the user to the latest test.

@ktpolanski
Copy link
Contributor

Let me know when you're happy with this

@slobentanzer
Copy link
Collaborator Author

I would be happy to merge. I have not used the automatic PR check, so I'd be interested to see how that works. For that we will need to merge and setup the procedure in the teichlab upstream. We can use another PR to test.

@ktpolanski
Copy link
Contributor

What needs to be done once this gets merged? I was under the impression that it would work as is, minus changing the URLs to badges and tests.

@slobentanzer
Copy link
Collaborator Author

Your impression is correct, I think we only need to update the URLs from slobentanzer to teichlab. I postponed this until the merge because I can't do troubleshooting on my branch any more once they are changed.

@ktpolanski ktpolanski merged commit 45da8a2 into Teichlab:master Jul 31, 2023
@slobentanzer
Copy link
Collaborator Author

@ktpolanski one more thing just occurred to me: you will need to update the Workflow permissions (repository settings, Actions, General) to allow the workflow action to "read and write" to make the coverage part of the Action functional.

@ktpolanski
Copy link
Contributor

Alright, merged, and then updated to Teichlab. The tests ran, the badge went from no status to passing. Success. Would it be possible to have this actually be a relative link? This way the badge/tests could be used for tracking in forks too.

The closest I found to "read and write" in the section you specified looks like this. So seems like it already does this?

image

@slobentanzer
Copy link
Collaborator Author

Would it be possible to have this actually be a relative link?

You can in principle have variables on the runner to account for the branch, but this complicates things quite a bit. Since I personally have the opinion that long running feature branches are bad practise, and that any single feature branch can be automatically checked via the PR when it wants to be merged, I would advise against that.

So seems like it already does this?

Yes, your settings are already correct. Note that it would have passed this time without the setting as well, since the coverage has not changed. The action needs the write permission only when it would actually push something, i.e., when the test coverage increases or decreases.

@ktpolanski
Copy link
Contributor

Yeah but ideally this would make a forker's life easy. They spin off a fork, they do some stuff, the badge updates to let them know that things are fine or not fine. Avoid a surprise once the PR is put in.

@ktpolanski
Copy link
Contributor

That said, I'm not sure that what I want is even possible. My relative link of notebooks/automatic_qc.ipynb becomes expanded to https://github.com/Teichlab/sctk/blob/master/notebooks/automatic_qc.ipynb while the tests don't have a /blob/master in there.

@slobentanzer
Copy link
Collaborator Author

I would rather advise contributors to just run the test suite locally. If they want to develop the package, they will need to do this in any case. This also makes it possible to test without pushing, which I find I often want to do.

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

2 participants