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

Fix multiple testing issues with the current GH Actions workflow and Composer scripts #274

Merged
merged 8 commits into from
Mar 16, 2022

Conversation

VadimSchmitz
Copy link
Contributor

@VadimSchmitz VadimSchmitz commented Mar 14, 2022

Fixed 5 issues and added 1 feature

  • Workflow now installs dependencies and tests them on the container instead on the vm.

  • Yarn audit was not working properly due to a bash quirk.

  • Xdebug is now installed and enabled so the code coverage tests work as intended.

  • Other security audit now actually works instead of exiting with 0 even when it didn't run.

  • Moved local-php-security-checker to its own bash file instead of having a script in composer.json.

  • Replaced PHPCPD with JSCPD, due to Xdebug^3.0 PHPCPD would no longer work and therefore it is not replaced.

Previously commands were not executed within the docker container but outside of it on the virtual environment of github. Now all the tests will be done on the actual container
@VadimSchmitz VadimSchmitz requested a review from MKodde March 14, 2022 14:10
@VadimSchmitz VadimSchmitz changed the title Fix multiple issues with the current GH Actions workflow Fix multiple testing issues with the current GH Actions workflow and Composer scripts Mar 15, 2022
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

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

Great work again. I love seeing this taking shape!

See some feedback in the comments below.

.github/workflows/test-integration.yml Outdated Show resolved Hide resolved
@@ -44,7 +43,7 @@ jobs:
- name: Build frontend assets
run: cd ci/docker && docker-compose exec -T stepup-ra bash -c 'source "/usr/local/nvm/nvm.sh" && yarn encore production'
- name: Run test scripts
run: cd ci/docker && docker-compose exec -T stepup-ra bash -c ' composer test '
run: cd ci/docker && docker-compose exec -T stepup-ra bash -c ' composer test && source "/usr/local/nvm/nvm.sh" && yarn audit '
Copy link
Member

Choose a reason for hiding this comment

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

I guess this commit can be dropped. As the source issue was later resolved.


"wget -q https://github.com/fabpot/local-php-security-checker/releases/download/v1.0.0/local-php-security-checker_1.0.0_linux_amd64 -O local-php-security-checker && chmod +x ./local-php-security-checker && ./local-php-security-checker",
source "/usr/local/nvm/nvm.sh"
yarn audit
Copy link
Member

Choose a reason for hiding this comment

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

Give'er a linefeed will ya?

ci/qa/phpunit Show resolved Hide resolved
ci/qa/security-tests Show resolved Hide resolved
ci/qa/security-tests Show resolved Hide resolved
Comment on lines 6 to 8
defaults:
run:
shell: bash -l {0}
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed, that was part of my R&D work.

Comment on lines 6 to 8
stepup-ra:
stdin_open: true
tty: true
Copy link
Member

Choose a reason for hiding this comment

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

Please try removing this option. I do not believe it being critical.

.github/workflows/test-integration.yml Show resolved Hide resolved
printf 'Curl failed downloading php-security-checker with error code "%d"\n' "$?" >&2
exit 1
fi
source "/usr/local/nvm/nvm.sh"
Copy link
Member

Choose a reason for hiding this comment

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

One final leftover that can now be removed as per bash -l

by adding the -l flag to the docker exec command there is no longer a
need to source for yarn each time it is called
@MKodde
Copy link
Member

MKodde commented Mar 15, 2022

Inspecting the GHA output. I see Sebastian CPD throwing a fatal error.

PHPCPD was no longer functioning due to Xdebug 3 not supporting the version of PHPCPD we are using. PHPCPD could not be updated due to the projects PHP version not allowing it. Therefore there is chosen to replace PHPCPD with JSCPD.
@VadimSchmitz
Copy link
Contributor Author

With the newest commit PHPCPD has been replaced with JSCPD, See latest commit for an explanation.

The correct folders are now ignored, a limit of 30 duplicates is set for
triggering a duplication warning.

Breaching a project wide duplication treshold of 1% will start causing
the copy paste detector to exit with a 1 status code
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

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

Great work, I'm happy we found a good PHP CPD replacement. I'm not often happy about adding a JS dependency, but this is a great example.

@VadimSchmitz VadimSchmitz merged commit cdbe759 into develop Mar 16, 2022
@VadimSchmitz VadimSchmitz deleted the bugfix/tests branch March 16, 2022 13:42
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.

2 participants