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

Added initial bats tests and github action set up #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gaol
Copy link

@gaol gaol commented Sep 24, 2021

This PR tries to add initial bats tests for Hera and set up github action CI.

  • tests/tests-common.sh - based on the one in harmonia
  • tests/tests-suite.sh - based on the one in harmonia

tests/run.sh-tests.bats Outdated Show resolved Hide resolved
Copy link
Collaborator

@spyrkob spyrkob left a comment

Choose a reason for hiding this comment

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

couple small questions, otherwise LGTM... at least for my bash-fu ;)

tests/tests-suite.sh Outdated Show resolved Hide resolved
tests/tests-common.sh Outdated Show resolved Hide resolved
tests/tests-common.sh Outdated Show resolved Hide resolved
@gaol
Copy link
Author

gaol commented Sep 28, 2021

Feedback on the review:

  • Updated tests-suite.sh accordingly to fail the github action when either bats test fails or ShellCheck violations are found.
  • Updated job.sh to fix ShellCheck violation of SC2145
  • Added a .shellcheckrc file to exclude some ShellCheck rules to pass the CI for the moment. I am not sure how to solve the violations, as currently all work well, I think this should be OK to ignore currently.

@@ -46,4 +46,4 @@ run_ssh "podman exec \
-e PULL_REQUEST_PROCESSOR_HOME="${PULL_REQUEST_PROCESSOR_HOME}" \
-e VERSION="${VERSION}" \
-e COMPONENT_UPGRADE_LOGGER="${COMPONENT_UPGRADE_LOGGER}" \
-ti ${CONTAINER_NAME} '${BUILD_SCRIPT}' ${@}"
-ti ${CONTAINER_NAME} '${BUILD_SCRIPT}' $*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be OK, but @rpelisse could you confirm this?

Choose a reason for hiding this comment

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

@spyrkob @gaol I'm not sure about this change, I've never seen $* before... What is the reason for this change? What does $* that differs from ${@} ? In any case, I think it should be ${*}.

@spyrkob
Copy link
Collaborator

spyrkob commented Sep 28, 2021

Thanks @gaol!

I think running shellcheck with -x would solve SC1091 (ie allow reading linked files)
The rest of violations seem to be valid points (AFAICT), we should address those, but it can be separate PR.

@@ -46,4 +46,4 @@ run_ssh "podman exec \
-e PULL_REQUEST_PROCESSOR_HOME="${PULL_REQUEST_PROCESSOR_HOME}" \
-e VERSION="${VERSION}" \
-e COMPONENT_UPGRADE_LOGGER="${COMPONENT_UPGRADE_LOGGER}" \
-ti ${CONTAINER_NAME} '${BUILD_SCRIPT}' ${@}"
-ti ${CONTAINER_NAME} '${BUILD_SCRIPT}' $*"

Choose a reason for hiding this comment

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

@spyrkob @gaol I'm not sure about this change, I've never seen $* before... What is the reason for this change? What does $* that differs from ${@} ? In any case, I think it should be ${*}.

done
exit 1
fi
echo 'Done - PASSED'

Choose a reason for hiding this comment

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

@gaol There seems to be quite a lot of code duplication from Harmonia's testsuite... I wonder if it would no be cleaner to have Jenkins checkout Harmonia inside the Hera folder and reuse the stuff... WDTK?

@rpelisse
Copy link

rpelisse commented Mar 6, 2024

@gaol where are we on this PR? It seems to have gone stale, can you rebase and update?

@gaol
Copy link
Author

gaol commented Mar 7, 2024

Ah, it must had been lost in my list 😁 will update it soon

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