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

test: add workflows #33

Closed
wants to merge 32 commits into from
Closed

test: add workflows #33

wants to merge 32 commits into from

Conversation

ianlnf
Copy link
Contributor

@ianlnf ianlnf commented Mar 29, 2023

  • Adds a separate workflow file for each test
  • Improves test folder structure
  • Reinstates label tests

Resolves #32

Note that the tests occasionally fail in CI, so the tests have been temporarily disabled in the CI workflow. Issue #34 has been raised to review the Docker container error. The tests do run locally.

@ianlnf ianlnf marked this pull request as ready for review March 29, 2023 15:20
@ianlnf ianlnf removed the test-log label Mar 29, 2023
@simoneb
Copy link
Member

simoneb commented Mar 30, 2023

@ianlnf are you looking for a review on this? if so, fix CI first please

Base automatically changed from test/jest to master March 30, 2023 09:55
@ianlnf
Copy link
Contributor Author

ianlnf commented Mar 30, 2023

@ianlnf are you looking for a review on this? if so, fix CI first please

Updated now, ready for review. The logging added shows that Act tests do fail occasionally in CI with a Docker container error, but run locally. I've disabled the tests in CI and added issue #34. Options are removing the tests (no automated testing) or resolving the issue. But I think either option should be done after this PR is merged, as it has improved test structure anyway. The local tests may be of some value.

@simoneb
Copy link
Member

simoneb commented Mar 30, 2023

My recommendation is to ditch act altogether and write unit test as we did in all out other actions. Are you up for picking this up?

@ianlnf
Copy link
Contributor Author

ianlnf commented Mar 30, 2023

My recommendation is to ditch act altogether and write unit test as we did in all out other actions. Are you up for picking this up?

This is a purely composite action, there is no custom code to unit test

@simoneb
Copy link
Member

simoneb commented Mar 30, 2023

My recommendation is to ditch act altogether and write unit test as we did in all out other actions. Are you up for picking this up?

This is a purely composite action, there is no custom code to unit test

That's exactly the idea. Logic should live in code, not in yaml. I'd turn this into a JS action, ditch act and write pure unit tests for the logic.

@ianlnf
Copy link
Contributor Author

ianlnf commented Apr 3, 2023

My recommendation is to ditch act altogether and write unit test as we did in all out other actions. Are you up for picking this up?

This is a purely composite action, there is no custom code to unit test

That's exactly the idea. Logic should live in code, not in yaml. I'd turn this into a JS action, ditch act and write pure unit tests for the logic.

I still think there is value in pursuing an effective mechanism for developing integration tests with composite actions longer term. Their raison d'etre is the reuse of logic within YAML, allowing existing actions to be repurposed to produce custom behaviour efficiently. Even if this action were to be refactored to JS/TS, which arguably is unnecessary here, it may still remain a composite action with custom code via github-script, e.g. if it wished to reuse actions such as input validation, copying of files, checkout etc. Testing only custom code in this case with unit tests may be sufficient, but may not be ideal. Integration tests could offer broader validation and less coupling to the production code.

I think either way though, this PR could be merged and issue #34 assessed for effort vs a refactor. It may be a configuration error or an issue that may be resolved in future Act versions. There is an issue with duplicate Act job names that can be reviewed. I do think the act-js API can be improved and offer additional features such as mocking of composite action steps and inputs. This would reduce the boilerplate and remove the need to check for env.ACT within the action metadata.

@simoneb
Copy link
Member

simoneb commented Apr 14, 2023

I'm going to close this PR as it will be difficult to hand over to somebody else.

@simoneb simoneb closed this Apr 14, 2023
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.

Review Act tests in CI
2 participants