[#60911] Rehabilitate frontend component test suite #17694
Conversation
myabc
force-pushed
the
feature/github-workflow-test-frontend-unit
branch
7 times, most recently
from
680e580 to
84318d5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...s/formattable-textarea-input/components/formattable-control/formattable-control.component.ts
Fixed
Show fixed
Hide fixed
...c/app/shared/components/dynamic-forms/services/dynamic-fields/dynamic-fields.service.spec.ts
Fixed
Show fixed
Hide fixed
myabc
force-pushed
the
feature/github-workflow-test-frontend-unit
branch
2 times, most recently
from
8fc66c7 to
e91d371
Compare
As of 5a2a501 we are now importing the actual I18n implementation. Also resolves the following warning: Unable to determine file type from the file extension, defaulting to js.
Allows us to perform plugin symlinking and file generation necessary for bootstapping frontend tests. Based on a standard plugin setup.
Broken with upgrade from v5 to v6. Formly now defines an extra `formly-field` for a given `formly-form` component. See: https://formly.dev/docs/guide/migration#10-formly-root-field
Fixes additional schema warnings.
Updates spec for selector change in 077112d.
Updates spec for store, selector changes in 077112d. Also forces change detection via `ChangeDetectorRef`.
myabc
force-pushed
the
feature/github-workflow-test-frontend-unit
branch
from
e91d371 to
4d8bccc
Compare
| - '**/frontend/**/*.ts' | ||
| - '**/frontend/**/*.js' | ||
| - '**/frontend/**/*.json' |
There was a problem hiding this comment.
It would be great if someone could check these filter patterns. I was caught out at first by GH Actions lack of support for extended globbing (see [Filter pattern cheat sheet).
myabc
changed the title
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '18.13' |
There was a problem hiding this comment.
I specified 18.13 because it's the same version we're using in eslint-core.yml. However we should probably switch both to 20.9.0, the node version specified in docker/ci/Dockerfile, as well as the root package.json in the repo.
| jobs: | ||
| units: | ||
| name: Units | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
ubuntu-latest resolves to GitHub's Ubuntu 24.04 runner image, which includes Chrome, chromedriver amongst other browsers and drivers.
There was a problem hiding this comment.
It might be better to explicitly specify ubuntu-24.04:
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
I copied and pasted this from eslint-core.yml, but I think this isn't what we want - according to the docs this fetches all history for all tags and branches. / cc @machisuji
|
|
||
| - name: Test (Angular) | ||
| id: npm-test | ||
| run: npm test -- --code-coverage |
There was a problem hiding this comment.
Although test coverage results are written to stdout (so visible in the job output), we should probably find a way to append the report to the PR somehow - @machisuji @cbliard @crohr any ideas what our options might be?
| 'node_modules/jquery/dist/jquery.js', | ||
| // 'node_modules/angular-mocks/angular-mocks.js' | ||
| ], | ||
| frameworks: ['jasmine', '@angular-devkit/build-angular'], | ||
| plugins: [ | ||
| require('karma-jasmine'), | ||
| require('karma-chrome-launcher'), | ||
| require('karma-coverage'), |
There was a problem hiding this comment.
From glancing at the source for @angular-devkit/build-webpack, ng test should append this plugin automatically if "test"/"options"/"codeCoverage": true is specified. However I couldn't get it to work.
| // This is a very project resource specific hack to allow macros on description and statusExplanation but | ||
| // disable it for custom fields. As the formly based approach is currently limited to projects, and that is to be removed, | ||
| // such a "pragmatic" approach should be ok. | ||
| macros: (this.templateOptions.property as string).startsWith('customField') ? 'none' : 'resource', | ||
| options: { rtl: this.templateOptions?.rtl }, | ||
| macros: this.templateOptions.property?.startsWith('customField') ? 'none' : 'resource', |
There was a problem hiding this comment.
safe navigation operator is necessary here!
| payloadValue?:{ href?:string|null }; | ||
| rtl?:boolean; | ||
| locale?:string; | ||
| bindLabel?:string; | ||
| bindValue?:string; | ||
| searchable?:boolean; | ||
| editorType?:ICKEditorType; | ||
| noWrapLabel?:boolean; | ||
| virtualScroll?:boolean; | ||
| clearOnBackspace?:boolean; | ||
| clearSearchOnAdd?:boolean; | ||
| hideSelected?:boolean; | ||
| text?:Record<string, string>; | ||
| typeahead?:boolean; | ||
| inlineLabel?:boolean; | ||
| clearable?:boolean; | ||
| multiple?:boolean; |
There was a problem hiding this comment.
FormlyFieldConfig is now a generic interface with a Props type parameter. As stated in the associated commit, this could (and perhaps should) be reworked as a an interface hierarchy (+ discriminated unions), defining only the applicable properties for a given OPInputType or template type. Could possibly be done as part of op#60912.
| changeDetectorRef = fixture.debugElement.injector.get(ChangeDetectorRef) as jasmine.SpyObj<ChangeDetectorRef>; | ||
| // @ts-ignore | ||
| component.workPackage = { id: 'testId' }; | ||
|
|
||
| changeDetectorRef.markForCheck(); |
There was a problem hiding this comment.
manually invoking change detection gets this spec passing - not sure if this is the canonical way of testing asynchronous in Angular tests though?
| @@ -41,8 +41,8 @@ export class GitActionsService { | |||
| return str | |||
| .replace(/&/g, 'and ') // & becomes and | |||
| .replace(/ +/g, '-') // Spaces become dashes | |||
| .replace(/[\000-\039]/g, '') // ASCII control characters are out | |||
There was a problem hiding this comment.
\039 isn't a valid Octal - cool TS 5.5 validates regexen!
| @@ -4,6 +4,7 @@ import { | |||
| import { DOCUMENT } from '@angular/common'; | |||
| import { DomAutoscrollService } from 'core-app/shared/helpers/drag-and-drop/dom-autoscroll.service'; | |||
| import { findIndex, reinsert } from 'core-app/shared/helpers/drag-and-drop/drag-and-drop.helpers'; | |||
| import dragula from 'dragula'; | |||
There was a problem hiding this comment.
Need to double-check this - Dragula is required with the expose-loader in init-vendors.ts, which we're not currently importing in test.ts. We are also not importing typings/shims.d.ts.
Add stubs for ICKEditor: this works around an issue with the use of dynamic ESM imports in `CKEditorSetupService#load()` not working with our Karma/Webpack configuration.
Replaces deprecated `FormlyTemplateOptions` interface. See: https://formly.dev/docs/guide/migration#ngx-formlycorejson-schema N.B. That further improvements to type safety could be made by extending `IOPFormlyTemplateOptions`, employing discriminating unions.
Functionality introduced in TypeScript 5.5. See: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-5.html#inferred-type-predicates
TypeScript 5.5 introduces syntax checking for regexen. See: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-5.html#regular-expression-syntax-checking
Fixes schema warnings.
Relate to Angular router:
NG0303: Can't bind to 'uiSref' since it isn't a known property of 'a' (used in the 'ScrollableTabsComponent' component template).
NG0303: Can't bind to 'uiParams' since it isn't a known property of 'a' (used in the 'ScrollableTabsComponent' component template).
Match the `node-version` defined in root `package.json` `engines` entry, `.github/workflows/docker.yml` and `docker/ci/Dockerfile`.
myabc
force-pushed
the
feature/github-workflow-test-frontend-unit
branch
from
4d8bccc to
148c151
Compare
|
@myabc Feature tests are broken due to
|
Ticket
https://community.openproject.org/wp/60911 (part of Feature https://community.openproject.org/wp/60910)
What are you trying to accomplish?
Goals:
This is an alternative to #17677, which removes the frontend test suite entirely.
Screenshots
What approach did you choose and why?
This Pull Request initially started out as a spike - to see how easy it would be to restore CI runs for the suite (i.e. setting up a GitHub workflow action). However, once #17648 and #17646 were merged, only ~10 specs out of 450 were failing. As such, I went ahead and have attempted to fix these failures.
GitHub workflow for frontend component test suite
This is as a very simple, "vanilla" workflow action. It relies on the included software in GitHub's Ubuntu 24.04 runner image +
setup-node@v4action to setup the environment and execute the spec suite.The most significant bit of work was to port the Rake
openproject:plugins:register_frontendtask to a node script. This was done so that we don't need a step that sets up ruby, installs OpenProject dependencies.Things to note:
generate_plugin_sass, as AFAICT global styling isn't necessary for the frontend test suite to run successfully.Spec fixes
The failures in the frontend test suite are a result of the test suite having become out-of-sync with the actual implementation - which is understandable since it hasn't been running on CI for a number of years, and therefore overlooked.
The reasons for the failures and how I fixed them can basically be summarised as follows:
FormlyTemplateOptionswithFormlyFieldProps- however there is some additional work that needs to be done to properly deal with Formly 6.x deprecations. I have broken this out into separate work package, op#60912.CKEditorSetupService#load()use of dynamic ESM imports: I spent a fair amount of time playing around withangular.json/karma.config.jssettings, even going as far as debugging the Webpack config thatng testgenerates under-the-hood. However I struggled to get past "chunk load errors", so ended up replacing replacingCKEditorSetupService#create()with a spy that returns a stubICKEditorimplementation. I think this is probably a better solution going forward.To make things easier for review - I have referenced the commits that most likely caused the spec failures in several of this PR's commit messages. I'll also list the related PRs in the checklist below - it would be really fantastic if you could have a quick look at the associated spec changes.
Merge checklist
Added/updated documentation in Lookbook (patterns, previews, etc)fdescribe)Additional tasks (might be out-of-scope)
test-core.yml) workflow if frontend test suite check fails** I believe this is possible with
workflow_run, but @crohr / @machisuji probably know more? If it is technically possible, we should also figure out if introducing workflow dependencies makes sense (as part of op#60910), especially given that the frontend test suite does not currently test any non-Angular TS/JS code.