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.

Sign up for GitHub

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

Jump to bottom

Continuous Integration & Move inline shared commands to commands file #73

Merged
merged 1 commit into from
Oct 29, 2020
Merged

Continuous Integration & Move inline shared commands to commands file #73

merged 1 commit into from
Oct 29, 2020

Conversation

Lewiscowles1986
Copy link
Contributor

@Lewiscowles1986 Lewiscowles1986 commented Oct 26, 2020 •
edited
Loading

CI cypress for GitHub actions

Addresses #72 (CI tests, not linting)

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore improves repo, but is not bug or feature

Description

  • Move commands out of the only test case and into commands
  • Use Cypress GitHub action to test

Link(s)

Screenshot(s)

N/A

Checklist:

  • I have thoroughly read the CONTRIBUTING guidelines.
  • I understand my pull request will be thoroughly reviewed at high detail.
  • I understand the work in my pull request will only be available in the next version release of Checka11y.css and not in the current version release.
  • I confirm the work in this pull request is valid according to my findings and is not something for anything personal.
  • I have updated the README and/or features.md where and if applicable (still put an x if you have considered this but thought there was nothing to add or modify).
  • I have added myself to the contributors section in package.json (still put an x if you have considered this but decided not to add yourself).
  • I have checked I have not committed any accidental files.
  • I have tested all the main modern browsers (I.e. Chrome, Firefox, Edge, Safari - please leave this unchecked if there were any browsers listed you could not test and list them in the help section with details why you couldn't test that browser)
  • I have run the automated tests and added new ones to cover new code.
  • All new and existing a11y checks still work correctly (compare your local test/index.html to the test/index.html in the master branch).

Help

Clicking the actions tab, this should have run and passed when it's ready.

This also will not run on this repo from this PR. I don't actually know why that is, but I'm assuming once it merges, this will be run. It might be about preventing me as a non-contributor from running CI which might do things. It has run on a PR I raised against the forked repo. Passing CI

Additional work needed to guard a branch

  1. Click on repository settings
    image
  2. Click on Branches
    image
  3. Click Add rule
    image
  4. Specify a branch pattern, and select shown options
    image
  5. Click "Create" (you may be asked to authenticate)
    image

This will then ensure nobody can merge before CI passes, unless they are an admin (and it should be understood they DO NOT)

@Lewiscowles1986
Move commands to commands
fadfaaa 
These should be usable between tests

Moving these here and running the checks helps to verify this PR
@Lewiscowles1986 Lewiscowles1986 marked this pull request as ready for review October 26, 2020 17:37
@Lewiscowles1986 Lewiscowles1986 changed the title Move commands to commands Continuous Integration & Move commands to commands Oct 26, 2020
@Lewiscowles1986 Lewiscowles1986 mentioned this pull request Oct 26, 2020
Open
2 tasks
@Lewiscowles1986 Lewiscowles1986 changed the title Continuous Integration & Move commands to commands Continuous Integration & Move inline shared commands to commands file Oct 26, 2020
alvaromontoro
alvaromontoro approved these changes Oct 28, 2020
View reviewed changes
cypress/integration/a11y_spec.js
@@ -4,40 +4,6 @@ describe('CheckA11y.css Tests', () => {
const WARNING_BOXSHADOW = 'rgb(255, 255, 102) 0px 0px 0px 4px';
const ERROR_BORDER = '6.4px solid rgb(255, 0, 0)';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a show stopper, but should these constants be moved to a more global scope too?
Eventually, I think it could be good to break this file so each a11y check has its own spec file (even if it's with only 1-2 tests each). That way the tests would be separated by topic and we would potentially avoid many merge conflicts.

Copy link
Contributor Author

@Lewiscowles1986 Lewiscowles1986 Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that would be a good idea.

Strangely two show as unused only when I edit the file... I Grepped and searched around. No usages found.

Unlike the commands I moved; I don't know if these should live there or have a strong feeling about where they should move.

Happy to follow direction, but otherwise I'd like to squeak that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also keep in mind commands were including this file full of comments before. We have no such file for constants.

Copy link
Owner

Choose a reason for hiding this comment

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

@alvaromontoro Are you happy with this? 😊

This is awesome work you two! Thank you!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NP jack. @alvaromontoro if not fine, let me know where you would like the files located for the constants so I'm not left guessing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always happy with everything. Actually, I approved the changes yesterday :P

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, sorry, should have more clear. I'm very unfamiliar with Cypress and haven't had chance to fully check it out (100% of my time currently is on another project, which should finish on Saturday). I noticed you left a comment and wanted to make sure you didn't think an action was needed.

Thank you both! @Lewiscowles1986 I will merge your PR now and follow your instructions. 💪

@jackdomleo7 jackdomleo7 linked an issue Oct 28, 2020 that may be closed by this pull request
CI #72
Closed
jackdomleo7
jackdomleo7 approved these changes Oct 29, 2020
View reviewed changes
Copy link
Owner

@jackdomleo7 jackdomleo7 left a comment

Choose a reason for hiding this comment

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

Approved. @Lewiscowles1986 & @alvaromontoro have worked fantastically well on this! 👌

@jackdomleo7 jackdomleo7 added the project enhancement Enhancement to improving the overall project label Oct 29, 2020
@jackdomleo7 jackdomleo7 merged commit f531ce6 into jackdomleo7:master Oct 29, 2020
@jackdomleo7
Copy link
Owner

@Lewiscowles1986 This is all new to me, but I went with this configuration
image

@Lewiscowles1986
Copy link
Contributor Author

Yeah, my instructions were the bare-minimum case for guarding contributors, 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaromontoro alvaromontoro alvaromontoro approved these changes

@jackdomleo7 jackdomleo7 jackdomleo7 approved these changes

Assignees

@Lewiscowles1986 Lewiscowles1986

Labels
project enhancement Enhancement to improving the overall project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI
3 participants
@Lewiscowles1986 @jackdomleo7 @alvaromontoro