-
Notifications
You must be signed in to change notification settings - Fork 33
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
Introduce Angular tester #202
Conversation
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
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 @Dumluregn, that's a lot of good work!
To make GH Actions work with BrowserStack we probably need to add the appropriate secrets to the repo.
Please take care of that so we can test the workflow properly. Also, please remeber to use 1.4.0
version of karma-browserstack-launcher
. Otherwise parallel jobs will fail.
Versioning of the @angular/cli before version 6.x.x is a bit confusing. Version 1.x.x actually corresponds to all Angular versions before 6, and since we are officially supporting Angular 5, we should test it too. However there is no LTS for this version. The other problem is that I didn't manage to install it locally (unlike the newer versions), so it would require some strange workarounds to create a testing app. Therefore I propose to ignore it.
If we go this route, we should stop officially supporting Angular 5. If we can't test it, even locally, then we can't say we support it. Dropping support for Angular 5 doesn't seem like a big issue to me though but it might come at a cost of releasing new major version of ckeditor4-angular
.
Logic for getting the versions to test is really, really convoluted (mostly inherited from React tester with some quirks). We could replace it with npm view @angular/cli dist-tags - we have less control over tested versions then, but it would spare us a lot of code, so I think it's worth considering.
That makes sense but what do you mean by having less control over tested versions? We will always get latest version for each major version, right?
The GH workflow for this is almost identical as for React tester (only differences are React/Angular references), so it may be a candidate for a ckeditor4-workflows-common repo.
That's a good idea but shouldn't a custom GH action go to a separate repo? However, there is some other stuff that could go to ckeditor4-workflows-common
. Could _helpers
logic be used by both React and Angular integrations? Maybe logging module or the way the package versions are determined? Of course, these would be all follow-up task.
Some more things I noticed:
- Why do we need separate assets folder with copies of karma.conf.js and demo-form.component.ts? Can't we re-use existing ones?
- We use Travis CI to run simple
ng test && ng e2e
. Maybe we can migrate that to GHA as well and remove.travis.yml
? Once we do that we can also simplifykarma.conf.js
. - Default arguments are not used in
angular-tester
. Runningnpm run test:angular-tester
ornpm run test:angular-tester -- --browser Chrome
fails.
Besides I have a few thoughts regarding tests of ckeditor4-angular
in general. There is no clear distinction between unit tests and E2E tests. The only true unit tests are here. Other test files such as demo-form.component.spec.ts and simple-usage.component.spec.ts test main component indirectly and resemble E2E scenarios. On top of that we have app.e2e-spec.ts. This is quite confusing to me. Also, all E2E-like tests seem to be running on source files. There are no tests that would use actual library output. We already had issues because of that (see #191). Maybe we could introduce different approach to E2E tests that would run on library output? I've done something like that for React v2. Could this be a good candidate for a follow-up task?
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
There was no activity regarding this pull request in the last 14 days. We're closing it for now. Still, feel free to provide us with requested feedback so that we can reopen it. |
It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days. |
There was no activity regarding this pull request in the last 14 days. We're closing it for now. Still, feel free to provide us with requested feedback so that we can reopen it. |
This PR introduces Angular tester. The idea is pretty much the same as in ckeditor/ckeditor4-react#166, but it had to be adjusted due to a little bit more complicated way of creating Angular app. We are doing it by locally installing
@angular/cli
and using it to do the job for us, and we copy needed files after that. The set of files also varies between Angular versions (but TBH less that one could expect). It may look strange that the testing folder is created outside the integration folder - Angular doesn't allow creating nested projects easily and I didn't find a simple solution for that. Wrapping the whole integration into an additional folder just to make it work looks like a total overkill.I'd like to point out a few things:
README.md
after we got to the final solution.@angular/cli
before version6.x.x
is a bit confusing. Version1.x.x
actually corresponds to all Angular versions before 6, and since we are officially supporting Angular 5, we should test it too. However there is no LTS for this version. The other problem is that I didn't manage to install it locally (unlike the newer versions), so it would require some strange workarounds to create a testing app. Therefore I propose to ignore it.npm init -y
to create an initialpackage.json
file. We could just have one as an asset, but I didn't find copying it a more elegant solution (creating a new one OTOH ensures us we are up to date here).npm view @angular/cli dist-tags
- we have less control over tested versions then, but it would spare us a lot of code, so I think it's worth considering.React/Angular
references), so it may be a candidate for ackeditor4-workflows-common
repo.Closes #140.
Closes #206.