[Exploration] Simplify testing #544
Conversation
frankharkins
force-pushed
the
fh-simplify-testing
branch
from
bf17f02
to
a26999f
Compare
frankharkins
force-pushed
the
fh-simplify-testing
branch
2 times, most recently
from
4e762d7
to
c0a8da0
Compare
frankharkins
force-pushed
the
fh-simplify-testing
branch
from
c0a8da0
to
c2ed131
Compare
There was a problem hiding this comment.
(Mainly a comment than a review.)
Hi @frankharkins :) here a bit of my opinion about it, feel free to ignore or tell me if some part aren't relevant :)
-
This looks more than an Ecosystem 2.0 than just a refactoring. Does not be more easy to create a new project more than a branch ? In a new project we should be able to test it and create issue to make it leave and upgrade and not delete everything from the actual version and not be able to come back to the old one.
-
With this solution I'm afraid on a few thinks : We do not control what we are running anymore. If we are not running our own tox, I don't think we should host Actions run of not already validated project.
We do not launch our own Tox and we will not be able to help and adjust as we do today. Also lot of projects don't have tox file or have tox but don't have Actions to run it in a fresh env and so, help them to create or debugging a tox or Actions is far more complicate than ask them to create an ecosystem.json with few basics commands. -
If we go full Actions, we will loose the ability to run the Ecosystem from everywhere. The epic [EPIC] Create a GitHub Actions trigger to call wf in another project #490 will not be able to work (without lot of duplication code).
-
In the subject to help project maintainer, I have the feeling people hardly go to read the documentation outside the Readme. I don't see them going to check the actions file to see what is in the wf. And from now on I think the wf is harder to understand than a good doc with just a few commands.
-
I think if we put every logic in the Actions we should have the entire project in the actions and not have a python lib anymore at all. I don't think having an hybrid project would help us or anyone else understanding how does it works and how to improve it. Of course we can still have some scripts but we should delete the python package and have more an architecture around the Actions (example : https://github.com/peter-evans/create-pull-request).
I'm not against this evolution, to be honest when we started the project it was the solution I was thinking we will do at first, but I didn't have in mind what the Ecosystem will be and what it could be few years later.
I still thinks this solution could work :) but we need to put more time in it and not just a refactoring. We can do (almost) what ever we want with Actions (even API), so we should create a real new project and take our time to test it create issue about it.
I hope I wasn't too critical ^^' it's not what I attempted to
| # We upload a JSON artifact so the calling job can read the results. | ||
| # This is a bit ugly, but there doesn't seem to be a better way of doing it. | ||
| # See https://github.com/orgs/community/discussions/17245 for details. |
There was a problem hiding this comment.
Yes, using artifact is the classical way to do it :)
Normally you can just add the toml file as artifact and at the end just push it without any special treatment
| tox -epy39 | ||
| echo "PASS=true" >> $GITHUB_ENV |
There was a problem hiding this comment.
Hmm this means test who don't pass will kill the run and not go in the next step and so project will never be update with false test
|
|
||
| - name: Add results | ||
| run: | | ||
| python manager.py add_results_from_artifacts |
There was a problem hiding this comment.
If we do a full action workflow we should delete everything from python lib to not have an hybrid project
| matrix: | ||
| repo_name: ${{ fromJSON(needs.get_projects.outputs.repo_names) }} | ||
| test_type: [standard, stable, development] | ||
| uses: frankharkins/ecosystem/.github/workflows/ecosystem-test-project.yml@fh-simplify-testing |
There was a problem hiding this comment.
Use relatif path to not have future problem with branch and fork ;)
|
|
||
| - name: Run tests | ||
| run: | | ||
| tox -epy39 |
There was a problem hiding this comment.
What happen if the project only run with python 10 or python 8 ?
|
I'm closing this as we've decided not to go down this route. |
Summary
This explores a simpler testing process. Comments welcome; nothing set in stone yet.
Details and comments
The main changes are:
tox.inia. All testing logic lives in
ecosystem-test-project.yml. Ecosystem developers and Qiskit maintainers should only need to look at this one file.b. Another action (
ecosystem-batch-test-projects.yml) runs tests on all Ecosystem projects.See an example run and it's pull request.