Introduce unit tests for GH actions scripts #10533
Conversation
markus-hinsche
force-pushed
the
gha-scripts-unit-tests
branch
from
7846078 to
a3bf800
Compare
markus-hinsche
force-pushed
the
gha-scripts-unit-tests
branch
from
daa8163 to
f10e0be
Compare
markus-hinsche
requested review from
virtualroot and
RASADSA
and removed request for
a team
Makefile needs to point to another dir for these tests anyway
| def _get_is_external_and_dataset_repository_branch(): | ||
| is_external = os.environ["IS_EXTERNAL"] | ||
| dataset_repository_branch = os.environ["DATASET_REPOSITORY_BRANCH"] | ||
| if str(is_external).lower() in ("yes", "true", "t", "1"): |
There was a problem hiding this comment.
environment variables are always strings, aren't they?
| if str(is_external).lower() in ("yes", "true", "t", "1"): | |
| if is_external.lower() in ("yes", "true", "t", "1"): |
| @@ -254,10 +251,10 @@ def read_results(file: str) -> Dict[str, Any]: | |||
| return result | |||
|
|
|||
|
|
|||
| def push_results(file_name: str, file: str): | |||
| def _push_results(file_name: str, file: str): | |||
There was a problem hiding this comment.
| def _push_results(file_name: str, file: str): | |
| def _push_results(file_name: str, file: str) -> None: |
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: |
There was a problem hiding this comment.
do we even need the matrix? There will be always just one set to test right?
There was a problem hiding this comment.
I removed the matrix
| with: | ||
| poetry-version: ${{ env.POETRY_VERSION }} | ||
|
|
||
| - name: Load Poetry Cached Libraries ⬇ |
There was a problem hiding this comment.
don't we need an extra step to also install the dependencies? Fully relying on the cache seems risky
There was a problem hiding this comment.
🙈, good catch
markus-hinsche
force-pushed
the
gha-scripts-unit-tests
branch
from
119cf32 to
dc597b3
Compare
|
I thought about running it on every PR in the beginning. If it turns out to be annoying for others, we can restrict it of course |
|
🚀 A preview of the docs have been deployed at the following URL: https://10533--rasahq-docs-rasa-v2.netlify.app/docs/rasa |
|
@markus-hinsche Sounds good - probably worth making it a required check then, right? Optional failing CI steps tend to be confusing |
|
@wochinge Sure, making it required would be great. I am not sure if I have enough permissions to configure this in GitHub though. Could you set the check as required? |
Proposed changes:
test-gh-actionsin Makefile, specify categorydev-dependenciesand updatepoetry.lockPotential caveats (Feedback from reviewers encouraged):
sys.path.appendbecause.github/scripts/is not yet in thePYTHONPATH. Any ideas?rasa/, but not.github/scriptsStatus (please check what you already did):
black(please check Readme for instructions)