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

Add Github Actions for testing #4285

Merged
merged 33 commits into from Sep 21, 2019
Merged

Add Github Actions for testing #4285

merged 33 commits into from Sep 21, 2019

Conversation

hudochenkov
Copy link
Member

Which issue, if any, is this issue related to?

#4273

Is there anything in the PR that needs further explanation?

It's a mix of #4273 (comment) and what Github gives by default for a Node.js project.

It need more work to migrate all features from .travis.yml.

@hudochenkov
Copy link
Member Author

I'm impressed by speed of Github CI. In 5m 18s it done 10 jobs (3 Node version * 3 OS + 1 lint job), while neither Appveyor (2 jobs) nor Travis (4 jobs) is done.

@hudochenkov
Copy link
Member Author

There is currently a bug with a step name:

Screenshot 2019-09-17 at 00 57 04

@hudochenkov
Copy link
Member Author

@hudochenkov
Copy link
Member Author

hudochenkov commented Sep 16, 2019

To-do:

I don't know why we use following env:

stylelint/.travis.yml

Lines 30 to 32 in c69e098

- node_js: '12'
script: npm run jest -- --maxWorkers=2
env: CI=tests 12

I don't know if we need this:

stylelint/.travis.yml

Lines 40 to 42 in c69e098

notifications:
webhooks:
- https://webhooks.gitter.im/e/d763493612da45967361

@digitaljohn is it something you can help us?

@jeddy3
Copy link
Member

jeddy3 commented Sep 17, 2019

We want to run CI on PRs (created PR, push to PR, force push). On master. And on

Yes, please.

Preferably to use the latest npm version for every OS/Node.js

Yep, we'll want this as we only support the latest npm version.

Add coveralls https://github.com/marketplace/actions/coveralls-github-action.

Nice

The Gitter hook can go. It's from a time when we used Gitter for support.

I can't remember what the following is for:

env: CI=tests 12 

What happens when it goes?

@hudochenkov
Copy link
Member Author

I noticed that all Windows jobs doesn't run Jest for some reason :( Only npm install.

@vankop
Copy link
Member

vankop commented Sep 17, 2019

I noticed that all Windows jobs doesn't run Jest for some reason :( Only npm install.

Do you have any log?

@hudochenkov
Copy link
Member Author

@vankop
Copy link
Member

vankop commented Sep 17, 2019

Straightforward idea can you write npm install && npm run jest ?

@vankop
Copy link
Member

vankop commented Sep 17, 2019

  • We want to run CI on PRs (created PR, push to PR, force push). On master. And on ^dependabot/.*$ branches.
on:
  push:
    branches:    
      - master               # Push events on master branch
      - 'dependabot/*'  # Push events on Dependabot branches
    paths:
      - '!*.md'
 pull_request:
   paths:
      - '!*.md'

Hope it is enough

@hudochenkov
Copy link
Member Author

@vankop feel free to work in this branch :)

@hudochenkov
Copy link
Member Author

I just realized that we don't use coveralls. We have it, but we have not badge or link in a readme. And to be honest, I don't to see comments like #4285 (comment) on Github and in my email. There are not helpful.

@hudochenkov
Copy link
Member Author

About using latest npm for all builds: actions/setup-node#31. It might not work, but we could try.

@hudochenkov
Copy link
Member Author

The last question to answer is to whether we need coveralls or not #4285 (comment). Other things are looking good! And we achieved everything we wanted and even more (like tests on macOS and Windows).

@hudochenkov
Copy link
Member Author

One more question to decide what are we going to do with Travis and Appveyor? I don't want to have them running with Github Actions at the same time. We don't need them anymore. When config files removed, then we have this results:

Screenshot 2019-09-20 at 01 29 27

I don't see a solution to keep using Travis and Appveyor for previously opened PRs, but not use them for future PRs and master. I propose disable them right before we merge this PR, and all previous PRs would need to be rebased on master. All PRs should be rebased anyway, because we changed formatting recently.

@hudochenkov
Copy link
Member Author

Tests still randomly fail on this test:

Screenshot 2019-09-20 at 01 40 17

But it's outside the scope of this PR. Restart helps.

@ntwb
Copy link
Member

ntwb commented Sep 20, 2019

I don't see a solution to keep using Travis and Appveyor for previously opened PRs, but not use them for future PRs and master. I propose disable them right before we merge this PR, and all previous PRs would need to be rebased on master. All PRs should be rebased anyway, because we changed formatting recently.

Agreed 👍🏼

I'd like to keep coveralls if we can, I think its still a useful tool for us

@hudochenkov
Copy link
Member Author

I'd like to keep coveralls if we can, I think its still a useful tool for us

If anyone uses it for stylelint? :) If not, then regardless of how useful it is, we can remove it.

@ntwb
Copy link
Member

ntwb commented Sep 20, 2019

If anyone uses it for stylelint? :) If not, then regardless of how useful it is, we can remove it.

I do partially, so even though we no longer have the coveralls badge on the readme I pipe the results into the Slack channel, I think it may still be configured to report on PRs if a PR changes the coverage by more than 5%, I think its a good safeguard to keep track of any potential issues we may run into by granting commit to any user who contributes.

image

@hudochenkov
Copy link
Member Author

Make sense. Is it possible to configure to not post a comment in every PR, but only after certain threshold?

@ntwb
Copy link
Member

ntwb commented Sep 20, 2019

Make sense. Is it possible to configure to not post a comment in every PR, but only after certain threshold?

That's the current behaviour I belive, it will only post a comment if the coverage changes by ~5%

p.s I just noticed we've hit 7,005 GitHub stars ⭐

@hudochenkov
Copy link
Member Author

This PR is ready for approvals :)

Copy link
Member

@jeddy3 jeddy3 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 everyone!

Looks fab to me.

@jeddy3 jeddy3 mentioned this pull request Sep 20, 2019
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Awesome 👍🏼👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants