-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#10874] Finalize Migration to Github Actions #10905
[#10874] Finalize Migration to Github Actions #10905
Conversation
Hi @rrtheonlyone, these parts of your pull request do not appear to follow our contributing guidelines:
|
Remove Appveyor as well? |
It actually makes more sense to separate into back-end, front-end, and E2E. It fits our paradigm of separation between back-end and front-end better. Add: although, it might not work for Codecov. If it really doesn't perhaps we should keep the status quo or use multiple code coverage services. |
I know coveralls supports merging different code coverage reports (used it recently for a project) but not sure about codecov. Maybe take a look at https://docs.codecov.io/docs/merging-reports |
Now that I think about it again, if we do go down the separate back end and front end workflow, then the coverage report needs to be separate as well otherwise it defeats the purpose. Of course, that's an if. |
So just to double check, our current options are:
|
Not really, keeping to status quo is never a considered option. It's either split to E2E + everything else (one coverage report, up to two build statuses) or split to BE/FE/E2E (two coverage reports, up to three build statuses). And as I mentioned before, if we do split BE/FE, it's more logical to also split the coverage reports. |
4de4ebd
to
8eca20c
Compare
Okay for now went with option (1) above. One coverage report + 2 build status: @damithc do we want to set it up such that on any change made to release branch, we run the E2E tests on multiple browsers and os? It is a simple change to do as well. |
We know production servers are Unix, so perhaps no need to run on other OS? Testing on multiple browsers might be nice to have. We can try it for a while to see how it goes, if you don't see any downsides. Are the tests equally stable on different browsers? |
This reasoning is not accurate. E2E goes beyond just testing back-end; the front-end should be tested and is very much dependent on user (which means it is virtually limitless). In fact, if we follow that line of reasoning, it is the back-end test that needs not be done on non-Unix. We still do it anyway because the cost is very low (and also to cater to Windows-based devs). But probably we should do it only on pushes to Speaking of cross-browser and cross-OS test, this is where services like Saucelabs will be useful for us. Probably it's about time to finish up #9597 for good. |
Oh right, I forgot that part. |
Okay guess we retain this for now. We run component tests + linting on windows/ubuntu and only run E2E on firefox/ubuntu at the moment. We can play around with more options in future. I can create a separate PR to add in multiple browsers + OS when we push to release branch. Last time when I checked chrome browser tests did fail more often. Ready for review |
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.
Also, do one last round of check that there is no more mention of "Travis" or "Appveyor" in the repo.
docs/process.md
Outdated
@@ -173,12 +173,12 @@ Their full descriptions can be viewed under the [labels page](https://github.com | |||
#### Code review | |||
|
|||
Your code will be reviewed, in this sequence, by: | |||
* Travis CI: by running static analysis.<br> | |||
* Github Actions: by running static analysis.<br> | |||
If there are problems found, the build will terminate without proceeding to testing.<br> |
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 don't think this line is accurate anymore; it was in V6
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.
yup removed
3fbc362
to
a5e7078
Compare
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.
Minor comments added. Almost there.
docs/development.md
Outdated
@@ -223,8 +223,10 @@ There are several files used to configure various aspects of the system. | |||
* `gradle.properties`, `gradle-wrapper.properties`: Contains the Gradle and Gradle wrapper configuration. | |||
* `package.json`: Contains the front-end third-party dependencies specification, as well as configurations for automated tasks/routines to be run via NPM. | |||
* `angular.json`: Contains the Angular application configuration. | |||
* `.travis.yml`: Contains the Travis CI job configuration. | |||
* `appveyor.yml`: Contains the AppVeyor CI job configuration. | |||
* `component.yml`: GitHub Action configuration for component tests run on every pull request/push to master or release branch. |
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.
"run on every push to master or release branch" is an additional detail that needs not be mentioned here. It also necessitates more places to be changed if we ever decide to change the practice.
- name: Start App Engine | ||
run: ./gradlew appengineStart | ||
- name: Start Tests | ||
run: ./gradlew lnpTests |
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.
Currently this will not pass in GitHub Actions without also incorporating the fixes from fb8177c.
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.
Ah I see, okay updated based on the commit
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.
LGTM
Part of #10874
build.gradle
:README