-
Notifications
You must be signed in to change notification settings - Fork 24
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
run CI with github actions #320
Conversation
merge from parent project
Implement GitHub actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this one! Had the plan to change from TravisCI to GitHub Actions for quite some time. Travis isn't working that well for tests that require different languages. Guess you had some "fun" reading that .travis.yml
.
I fear you missed some test cases. Guess due to the hard to read Travis configuration. Would be great if you could add them as well.
I'm a little bit confused that the GitHub Actions doesn't seem to run for this PR. Or are they just not reporting? Haven't worked that much yet with GitHub Actions to be honest. Do I have to activate something in repository config? Or do I just haven't looked at the correct places for CI reports?
Hey. Yeah me too. But it seems to need in master first. You can checkout my fork. There it was running. |
add additional tests
Hi @jelhan, I did the changes now. But the browserstack tests failed :/ When you merge this PR you first have to add
It can be also possible with github actions to automatically do releases and bundle them. That could be an additional feature in the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debugged the BrowserStack issue locally I think it's related to missing CI=true
environment variable. That one controls part of the build. It's set by default on TravisCI. That might cause the difference. It's also important for other tests cause otherwise we test with a build that doesn't include all babel transforms required to support legacy browsers.
When you merge this PR you first have to add
* `BROWSERSTACK_USERNAME` * `BROWSERSTACK_ACCESS_KEY` to the secrects of your github project. And you should change the password of your browserstack account. You should never have credentials within an opensource project in the source code!
If I remember correctly the credentials are included in code cause otherwise they are not available in build triggered by developers that aren't maintainers of the repo (read: pull requests by others). Are you sure that isn't an issue if using GitHub actions?
I would prefer to keep TravisCI for now and use both in parallel for some time. This would allow us to play around with GitHub Actions and keep TravisCI as a fallback if we face issues. Will remove later.
Can we run the tests for the frontend in parallel? Any chance to split up the jobs after installing dependencies? I've something in mind like the artifacts
feature of GitLab CI. Install dependencies in first stage and then run the different tests using the installed dependencies from first stage. We could even split the tests for different browsers and run them in parallel on a shared build. This would result in a tree like this:
yarn install
Install dependenciesyarn lint:js
yarn lint:hbs
yarn test:bundlesize
(uses a production build)yarn test:csp-header
yarn build --environment test
Build for test environment, requiresCI=true
environment variable.yarn test --launch Chrome --path dist
yarn test --launch Firefox --path dist
ember browserstack:connect
yarn test --launch BS_IE_11 --path dist --config-file testem.browserstack.js
yarn test --launch BS_MS_Edge --path dist --config-file testem.browserstack.js
yarn test --launch BS_Safari_Current --path dist --config-file testem.browserstack.js
If that's too much work for now or you aren't sure if that's even possible, we could also investigate that one separately.
I wasn't aware that in current setup all tests require a fully working PHP environment due to including api in build. It's not necessary if the build is only used for testing. Especially not if it's only for CI. This small change disables the inclusion of API for diff --git a/lib/include-api-in-build/index.js b/lib/include-api-in-build/index.js
index 5dd4b16..0662105 100644
--- a/lib/include-api-in-build/index.js
+++ b/lib/include-api-in-build/index.js
@@ -20,6 +20,13 @@ module.exports = {
name: 'include-api-in-build',
postBuild(result) {
+ let environment = this.app.env;
+
+ // do not include app if build is for testing purposes only
+ if (environment === 'test') {
+ return;
+ }
+
let outputPath = result.directory + '/api';
return Promise.resolve() Hope that makes it easier for you. |
Co-Authored-By: Jeldrik Hanschke <jelhan@users.noreply.github.com>
If I read the docs of
Using
If we are lucky this also fixes the issue of testem not finding |
Hey, happy new year :) Nice worked. |
Happy new year! Thanks a lot for keep working on this one. Guess you haven't expected it to be that much work, have you? BrowserStack seems to be a tricky beast. Not sure but maybe it's caused by having the establishing of the tunnel and the test run in two separates commands? Did you tried to do it in one command? - name: Test in IE11
run: yarn test:browserstack:connect && yarn test:browserstack-ie11 && yarn test:browserstack:disconnect |
No I haven't expected that! I just thought I am interested in github actions 😆 I think I give up. I have no insight on what is tested and where the issue could be. See the logs of the error here:
|
I can totally understand! But let me propose another solution which might not be that disappointed. I think you have already finished > 95% of the work. Lets get that one in! All tests are working except BrowserStack if I see it correctly. Maybe we just disable/remove that ones and still run them on TravisCI? We would be able to merge everything else and come back to that ones later. If you don't have the time for it anymore, I can take over and do it myself. But I think it may be more satisfying to you to get your work merged directly. Let me know which option you prefer. |
ok yeah sure, good idea. I will try to do it within the next week |
@jelhan I removed the browserstack tests now and readded yaml tests. |
@masterwendu Thanks a lot for working on this! |
#319
My first try with github actions.
On every pull request and on pushed to master it runs the frontend and backend tests in parallel.
Closes #303