ckeditor / ckeditor4-angular

Official CKEditor 4 Angular component.
Other
54 stars 32 forks source link

Introduce Angular tester #202

Closed Dumluregn closed 2 years ago

Dumluregn commented 3 years ago

This PR introduces Angular tester. The idea is pretty much the same as in https://github.com/ckeditor/ckeditor4-react/pull/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:

Closes #140. Closes #206.

github-actions[bot] commented 3 years ago

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.

github-actions[bot] commented 3 years ago

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.

Dumluregn commented 3 years ago

First of all, thank you for the thorough review and a wider look at the whole repo - you've mentioned a lot of things that indeed need some love.

Since I'm going to work on another project for some time I didn't manage to cover all the requested changes, but I've done my best to indicate what is already done and what not. Ping me if anything needs to be clarified when the work on this PR is resumed by someone else.

As for the comments:

Also, please remeber to use 1.4.0 version of karma-browserstack-launcher. Otherwise parallel jobs will fail.

πŸ‘πŸ»

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.

Mentioned in #209.

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?

Yeah, I couldn't really find a case where it would be useful to go the hard way and calculate the required versions on our own. So I've changed the implementation to use dist-tags. This way we are testing all LTS versions, current and next ones, which makes sense for me.

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?

karma.conf.js has some unnecessary stuff removed and e.g. simplified logging (to improve catching errors), so it's a bit different than the one in the repo. Also demo-form.component.ts has one line modified for older Angular versions (namely @ViewChild( 'demoForm' ) demoForm?: NgForm;) and I've figured that it's more elegant to just provide a separate file instead of messing with replacing the content with regexes.

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 simplify karma.conf.js.

I thought about it too, I'm just not sure if it shouldn't rather be done as a follow-up.

Default arguments are not used in angular-tester. Running npm run test:angular-tester or npm run test:angular-tester -- --browser Chrome fails.

It was indeed broken, sorry and thanks for bringing my attention to it πŸ‘πŸ»

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.

Indeed the test coverage in this repo is not very intuitive. What is more, Angular is going to drop Protractor (https://github.com/ckeditor/ckeditor4-angular/issues/206). There is plenty of other issues with them (such as self-dependency, as mentioned in https://github.com/ckeditor/ckeditor4-angular/issues/93), so it would make sense to rethink them. I've created a follow-up: https://github.com/ckeditor/ckeditor4-angular/issues/210.

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?

Yes, and it's even already reported: #181 πŸ‘πŸ»

Things to be done:

github-actions[bot] commented 3 years ago

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.

github-actions[bot] commented 2 years ago

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.

github-actions[bot] commented 2 years ago

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.

github-actions[bot] commented 2 years ago

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.

github-actions[bot] commented 2 years ago

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.

github-actions[bot] commented 2 years ago

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.

github-actions[bot] commented 2 years ago

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.

github-actions[bot] commented 2 years ago

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.