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

VAGOV-2142: Add yaml-tests to include phpunit, phpcs, and linting #348

Merged
merged 61 commits into from
Jun 8, 2019

Conversation

jonpugh
Copy link
Contributor

@jonpugh jonpugh commented May 24, 2019

This PR includes a new library I created to make maintaining and running a list of tests really easy. It also integrates directly with GitHub so each test is passed back to the PR as a commit status.

Simply composer install

Then composer yaml-tests or just composer y

Thew way yaml tests work, if there is a GITHUB_TOKEN environment variable, it will not only run the tests but post to the commit status API with the test pass or fail.

@jonpugh jonpugh self-assigned this May 24, 2019
@jonpugh jonpugh requested review from mbenziane and ElijahLynn May 24, 2019 20:53
@jonpugh
Copy link
Contributor Author

jonpugh commented May 24, 2019

So, this PR is the test itself...

The yaml-tests command can be run from anywhere, as long as it has a GITHUB_TOKEN it can post commit status back here...

From my local laptop I am about to run composer y --github-token=exxxx

@jonpugh
Copy link
Contributor Author

jonpugh commented May 24, 2019

The tests are defined in a tests.yml file:

va/phpunit:
  description: PHPUnit
  command: phpunit tests/phpunit --colors=always
va/phpcs:
    - phpcs --standard=PSR2 -n docroot/modules/custom docroot/themes --colors
va/phplint:
    - find docroot/modules/custom docroot/themes -name '*.php' -print0 | xargs -0 -n1 php -l
    - find docroot/modules/custom docroot/themes -name '*.module' -print0 | xargs -0 -n1 php -l
    - find docroot/modules/custom docroot/themes -name '*.install' -print0 | xargs -0 -n1 php -l

@va-cms-bot
Copy link
Collaborator

Created an environment test-vagovcms#pr-348

button_visit-site

@jonpugh
Copy link
Contributor Author

jonpugh commented May 24, 2019

The plugin includes an option for --test-file so we can specify a totally separate file for production tests.

docroot/.htaccess Outdated Show resolved Hide resolved
@ElijahLynn
Copy link
Contributor

Nice work @jonpugh, this is highly useful and powerful giving engineers the ability to control which tests are run (and on a PR level granularity) and love the status feedback here on the PR, so much more useful than a global pass/fail.

@va-cms-bot
Copy link
Collaborator

Created an environment test-vagovcms#pr-348

button_visit-site

@va-cms-bot
Copy link
Collaborator

Created an environment test-vagovcms#pr-348

button_visit-site

@jonpugh
Copy link
Contributor Author

jonpugh commented May 30, 2019

Ok!

I've got PHPCS and Linting now working!

Good new on another front: I swapped out the pre-commit hook to just call composer va:test:cs, so now the automated tests from yaml-test and the git commit pre-hook are running exactly the same command.

The pre-commit git hook file had additional logic to skip files, and other things. That logic is now in the composer command va:test:cs, including ignoring certain files.

@va-cms-bot
Copy link
Collaborator

Created an environment devops-vagovcms#pr-348

button_visit-site

@ElijahLynn
Copy link
Contributor

ElijahLynn commented Jun 7, 2019

W00t w00t!

image

@jonpugh
Copy link
Contributor Author

jonpugh commented Jun 7, 2019 via email

@jonpugh
Copy link
Contributor Author

jonpugh commented Jun 7, 2019

Incredible!
Thanks so much for taking this over the line, @ElijahLynn . The last few problems were DevOps side, it was hard for me to debug workout being now familiar.

1 similar comment
@jonpugh
Copy link
Contributor Author

jonpugh commented Jun 7, 2019

Incredible!
Thanks so much for taking this over the line, @ElijahLynn . The last few problems were DevOps side, it was hard for me to debug workout being now familiar.

@jonpugh
Copy link
Contributor Author

jonpugh commented Jun 7, 2019

The last steps as I see it:

  • Add --hostname= option back to the script. That will change the "aa21xxx" to be the url of the environment.
  • merge DevOps repo branch.
  • change repo ref back to master. Push, wait for another PASS
  • once pass happens on master DevOps repo, we should tag a release of that and put the tag in VAGOV___GIT_REF variable.

@ElijahLynn
Copy link
Contributor

The last steps as I see it:

  • Add --hostname= option back to the script. That will change the "aa21xxx" to be the url of the environment.

I don't want to hold up this PR because of the hostname. And we can't just add it back as it broke docker last night because it was too long, so we need more time to figure out how to add a hostname longer than 64 chars, unless you already know how. Do you see this as a blocker for merge?

  • merge DevOps repo branch.
  • change repo ref back to master. Push, wait for another PASS
  • once pass happens on master DevOps repo, we should tag a release of that and put the tag in VAGOV___GIT_REF variable.

@jonpugh
Copy link
Contributor Author

jonpugh commented Jun 7, 2019

rebuild

@ElijahLynn
Copy link
Contributor

@beeyayjay The MigrationCount failed just now. Should that test be disabled or kept up to date?
05317f2

@ElijahLynn
Copy link
Contributor

rebuild

@va-cms-bot
Copy link
Collaborator

Created an environment devops-vagovcms#pr-348

button_visit-site

@va-cms-bot
Copy link
Collaborator

Created an environment devops-vagovcms#pr-348

button_visit-site

@ElijahLynn ElijahLynn removed the DevOps CMS team practice area label Jun 7, 2019
@ElijahLynn
Copy link
Contributor

failed on performance test, I think there was something up with Jenkins slave container host on that one. The time to post status here was really long after the Ansible task started.

@ElijahLynn
Copy link
Contributor

build

@ElijahLynn
Copy link
Contributor

failed on performance test, I think there was something up with Jenkins slave container host on that one. The time to post status here was really long after the Ansible task started.

Almost feel like before any performance test, there needs to be a system check for CPU and Disk I/O that isn't dependent on the app and just tests the raw system.

@ElijahLynn
Copy link
Contributor

failed on performance test, I think there was something up with Jenkins slave container host on that one. The time to post status here was really long after the Ansible task started.

Come to think of it, the Jenkins master was displaying a "Jenkins is going to shutdown" message due to a scheduled restart so I do wonder if that had something to do with it.

@va-cms-bot
Copy link
Collaborator

Created an environment test-vagovcms#pr-348

button_visit-site

@ElijahLynn
Copy link
Contributor

Huzzah! Merging, GREAT WORK @jonpugh! 🚀 🎉 🎸

@ElijahLynn ElijahLynn merged commit 8db3ed8 into develop Jun 8, 2019
ElijahLynn pushed a commit to ElijahLynn/va.gov-cms that referenced this pull request Jun 10, 2019
…fairs#348.

Need this merge in master because deploys to STAGING and PROD are dependant on these.
ElijahLynn pushed a commit to ElijahLynn/va.gov-cms that referenced this pull request Jun 11, 2019
…fairs#348.

Need this merge in master because deploys to STAGING and PROD are dependant on these.
andyhawks added a commit that referenced this pull request Jun 11, 2019
VAGOV-2142 Manual cherry-pick of yaml-tests from #348 to master for PROD release
@ElijahLynn ElijahLynn deleted the VAGOV-2142-tests branch September 4, 2019 21:43
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.

4 participants