diff --git a/README.md b/README.md index 0712bdaae..14508c7ab 100644 --- a/README.md +++ b/README.md @@ -17,14 +17,16 @@ take up to 60 seconds once the docker build finishes. ### First steps - Install git commit template: [Commit Template](docs/commit.template.md). -- Volta: [Volta](#volta) +- [Volta](#volta) ### Recommended -- Compodoc: [Compodoc Conventions](docs/compodoc.md). -- Docker Commands: [Docker Commands](docs/docker.md). -- Git Conventions: [Git Conventions](docs/git-convention.md). -- NGXS: [NGXS Conventions](docs/ngxs.md). +- [Compodoc Conventions](docs/compodoc.md). +- [Docker Commands](docs/docker.md). +- [ESLint Strategy](docs/eslint.md). +- [Git Conventions](docs/git-convention.md). +- [NGXS Conventions](docs/ngxs.md). +- [Testing Strategy](docs/testing.md). ### Optional diff --git a/docs/eslint.md b/docs/eslint.md new file mode 100644 index 000000000..5a4933835 --- /dev/null +++ b/docs/eslint.md @@ -0,0 +1,105 @@ +# Linting Strategy – OSF Angular + +--- + +## Overview + +This project uses a **unified, modern ESLint flat config** approach to enforce consistent coding styles, accessibility standards, and TypeScript best practices. Linting runs on: + +- TypeScript (`*.ts`) +- HTML templates (`*.html`) +- Specs (`*.spec.ts`) + +It also integrates into the **Git workflow** via `pre-commit` hooks to ensure clean, compliant code before every commit. + +--- + +## Linting Commands + +```bash +# Run full project lint +npm run lint +``` + +--- + +## ESLint Config Structure + +### 1. TypeScript Files (`**/*.ts`) + +**Extends**: + +- `@eslint/js` → base ESLint config +- `typescript-eslint` recommended & stylistic configs +- `angular-eslint` TypeScript rules +- `eslint-plugin-prettier/recommended` + +**Plugins**: + +- `eslint-plugin-import` +- `eslint-plugin-simple-import-sort` +- `eslint-plugin-unused-imports` + +**Key Rules**: + +- Enforces Angular selector styles: + - Directives → `osfFoo` (camelCase) + - Components → `` (kebab-case) +- Enforces import sorting and duplicate prevention +- Removes unused imports automatically +- Allows unused variables named `_` to support convention (e.g., destructuring) + +--- + +### 2. HTML Templates (`**/*.html`) + +**Extends**: + +- `angular-eslint/templateRecommended` +- `angular-eslint/templateAccessibility` + +**Parser**: + +- `@angular-eslint/template-parser` + +**Key A11y Rules** (All Set to `"error"`): + +- `alt-text` +- `valid-aria` +- `click-events-have-key-events` +- `role-has-required-aria` +- `interactive-supports-focus` +- `no-distracting-elements` +- `no-autofocus` +- `label-has-associated-control` + +> This enforces **WCAG accessibility compliance** directly in Angular templates. + +--- + +### 3. Test Files (`**/*.spec.ts`) + +Loosened restrictions for developer convenience: + +```ts +'@typescript-eslint/no-explicit-any': 'off', +'@typescript-eslint/no-empty-function': 'off', +``` + +--- + +## Pre-Commit Hook + +The `pre-commit` file includes: + +- `npx lint-staged` → Lint only **staged files** before every commit +- Ensures **TypeScript and template linting** run automatically + +## Summary + +| File Type | Purpose | Key Rules | +| ------------ | ------------------------------------- | --------------------------------------- | +| `*.ts` | Lint Angular + TS logic | Unused imports, selector rules, sorting | +| `*.html` | Enforce a11y & Angular best practices | All template a11y rules enforced | +| `*.spec.ts` | Relaxed for test convenience | Some TS rules turned off | +| `pre-commit` | Git hook to lint before committing | Ensures consistent PR quality | diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 000000000..17448bdc9 --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,247 @@ +# OSF Angular Testing Strategy + +## Overview + +The OSF Angular project uses a modular and mock-driven testing strategy. A shared `testing/` folder provides reusable mocks, mock data, and testing module configuration to support consistent and maintainable unit tests across the codebase. + +--- + +## Index + +- [Best Practices](#best-practices) +- [Summary Table](#summary-table) +- [Test Coverage Enforcement (100%)](#test-coverage-enforcement-100) +- [Key Structure](#key-structure) +- [Testing Angular Services (with HTTP)](#testing-angular-services-with-http) +- [Testing Angular Components](#testing-angular-components) +- [Testing Angular Pipes](#testing-angular-pipes) +- [Testing Angular Directives](#testing-angular-directives) +- [Testing Angular NGXS](#testing-ngxs) + +-- + +## Best Practices + +- Always import `OsfTestingModule` or `OsfTestingStoreModule` to minimize boilerplate and get consistent mock behavior. +- Use mocks and mock-data from `testing/` to avoid repeating test setup. +- Avoid real HTTP, translation, or store dependencies in unit tests by default. +- Co-locate unit tests with components using `*.spec.ts`. + +--- + +## Summary Table + +| Location | Purpose | +| ----------------------- | -------------------------------------- | +| `osf.testing.module.ts` | Unified test module for shared imports | +| `mocks/*.mock.ts` | Mock services and tokens | +| `data/*.data.ts` | Static mock data for test cases | + +--- + +## Test Coverage Enforcement (100%) + +This project **strictly enforces 100% test coverage** through the following mechanisms: + +### Husky Pre-Push Hook + +Before pushing any code, Husky runs a **pre-push hook** that executes: + +```bash +npm run test:coverage +``` + +This command: + +- Runs the full test suite with `--coverage`. +- Fails the push if **coverage drops below 100%**. +- Ensures developers never bypass test coverage enforcement locally. + +> Pro Tip: Use `npm run test:watch` during development to maintain coverage incrementally. + +--- + +### GitHub Actions CI + +Every pull request and push runs GitHub Actions that: + +- Run `npm run test:coverage`. +- Verify test suite passes with **100% code coverage**. +- Fail the build if even **1 uncovered branch/line/function** exists. + +This guarantees **test integrity in CI** and **prevents regressions**. + +--- + +### Coverage Expectations + +| File Type | Coverage Requirement | +| ----------- | ------------------------------------------ | +| `*.ts` | 100% line & branch | +| `*.spec.ts` | Required per file | +| Services | Must mock HTTP via `HttpTestingController` | +| Components | DOM + Input + Output event coverage | +| Pipes/Utils | All edge cases tested | + +--- + +### Summary + +- **Zero exceptions** for test coverage. +- **Push blocked** without passing 100% tests. +- GitHub CI double-checks every PR. + +## Key Structure + +### `testing/osf.testing.module.ts` + +This module centralizes commonly used providers, declarations, and test utilities. It's intended to be imported into any `*.spec.ts` test file to avoid repetitive boilerplate. + +Example usage: + +```ts +import { TestBed } from '@angular/core/testing'; +import { OsfTestingModule } from 'testing/osf.testing.module'; + +beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [OsfTestingModule], + }).compileComponents(); +}); +``` + +### OSFTestingModule + +**Imports:** + +- `NoopAnimationsModule` – disables Angular animations for clean, predictable unit tests. +- `BrowserModule` – required for bootstrapping Angular features. +- `CommonModule` – provides core Angular directives (e.g., `ngIf`, `ngFor`). +- `TranslateModule.forRoot()` – sets up the translation layer for template-based testing with `@ngx-translate`. + +**Providers:** + +- `provideNoopAnimations()` – disables animation via the new standalone provider API. +- `provideRouter([])` – injects an empty router config for component-level testing. +- `provideHttpClient(withInterceptorsFromDi())` – ensures DI-compatible HTTP interceptors are respected in tests. +- `provideHttpClientTesting()` – injects `HttpTestingController` for mocking HTTP requests in unit tests. +- `TranslationServiceMock` – mocks i18n service methods. +- `EnvironmentTokenMock` – mocks environment config values. + +--- + +### OSFTestingStoreModule + +**Imports:** + +- `OSFTestingModule` – reuses core mocks and modules. + +**Providers:** + +- `StoreMock` – mocks NgRx Store for selector and dispatch testing. +- `ToastServiceMock` – injects a mock version of the UI toast service. + +### `testing/mocks/` + +Provides common service and token mocks to isolate unit tests from real implementations. + +**examples** + +- `environment.token.mock.ts` – Mocks environment tokens like base API URLs. +- `store.mock.ts` – NGXS or other store-related mocks. +- `translation.service.mock.ts` – Prevents needing actual i18n setup during testing. +- `toast.service.mock.ts` – Mocks user feedback services to track invocations without UI. + +--- + +### `testing/data/` + +Includes fake/mock data used by tests to simulate external API responses or internal state. + +Only use data from the `testing/data` data mocks to ensure that all data is the centralized. + +**examples** + +- `addons.authorized-storage.data.ts` +- `addons.external-storage.data.ts` +- `addons.configured.data.ts` +- `addons.operation-invocation.data.ts` + +--- + +--- + +## Testing Angular Services (with HTTP) + +All OSF Angular services that make HTTP requests must be tested using `HttpClientTestingModule` and `HttpTestingController`. + +### Setup + +```ts +import { HttpTestingController } from '@angular/common/http/testing'; +import { OSFTestingModule } from '@testing/osf.testing.module'; + +let service: YourService; + +beforeEach(() => { + TestBed.configureTestingModule({ + imports: [OSFTestingModule], + providers: [YourService], + }); + + service = TestBed.inject(YourService); +}); +``` + +### Example Test + +```ts +it('should call correct endpoint and return expected data', inject( + [HttpTestingController], + (httpMock: HttpTestingController) => { + service.getSomething().subscribe((data) => { + expect(data).toEqual(mockData); + }); + + const req = httpMock.expectOne('/api/endpoint'); + expect(req.request.method).toBe('GET'); + req.flush(getMockDataFromTestingData()); + + httpMock.verify(); // Verify no outstanding HTTP calls + } +)); +``` + +### Key Rules + +- Use `OSFTestingModule` to isolate the service +- Inject and use `HttpTestingController` +- Always call `httpMock.expectOne()` to verify the URL and method +- Always call `req.flush()` to simulate the backend response +- Add `httpMock.verify()` in each `it` to catch unflushed requests + +--- + +## Testing Angular Components + +- coming soon + +--- + +## Testing Angular Pipes + +- coming soon + +--- + +## Testing Angular Directives + +- coming soon + +--- + +## Testing NGXS + +- coming soon + +--- diff --git a/eslint.config.js b/eslint.config.js index 03e6b13dc..31c8a9a1a 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -6,6 +6,8 @@ const pluginImport = require('eslint-plugin-import'); const pluginSimpleImportSort = require('eslint-plugin-simple-import-sort'); const pluginUnusedImports = require('eslint-plugin-unused-imports'); const eslintPluginPrettierRecommended = require('eslint-plugin-prettier/recommended'); +const angularEslintTemplate = require('@angular-eslint/eslint-plugin-template'); +const angularTemplateParser = require('@angular-eslint/template-parser'); module.exports = tseslint.config( { @@ -24,7 +26,8 @@ module.exports = tseslint.config( 'unused-imports': pluginUnusedImports, }, rules: { - '@typescript-eslint/no-unused-vars': 'warn', + '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }], + '@angular-eslint/directive-selector': [ 'error', { @@ -42,10 +45,10 @@ module.exports = tseslint.config( }, ], 'import/first': 'error', - 'import/no-duplicates': 'warn', - 'import/newline-after-import': 'warn', + 'import/no-duplicates': 'error', + 'import/newline-after-import': 'error', 'simple-import-sort/imports': [ - 'warn', + 'error', { groups: [ // NGXS packages @@ -77,14 +80,35 @@ module.exports = tseslint.config( ], }, ], - 'simple-import-sort/exports': 'warn', - 'unused-imports/no-unused-imports': 'warn', + 'simple-import-sort/exports': 'error', + 'unused-imports/no-unused-imports': 'error', }, }, { files: ['**/*.html'], + plugins: { + '@angular-eslint/template': angularEslintTemplate, + }, + languageOptions: { + parser: angularTemplateParser, + }, extends: [...angular.configs.templateRecommended, ...angular.configs.templateAccessibility], - rules: {}, + rules: { + '@angular-eslint/template/banana-in-box': ['error'], + '@angular-eslint/template/eqeqeq': ['error'], + '@angular-eslint/template/no-negated-async': ['error'], + '@angular-eslint/template/alt-text': ['error'], + '@angular-eslint/template/click-events-have-key-events': ['error'], + '@angular-eslint/template/elements-content': ['error'], + '@angular-eslint/template/interactive-supports-focus': ['error'], + '@angular-eslint/template/label-has-associated-control': ['error'], + '@angular-eslint/template/mouse-events-have-key-events': ['error'], + '@angular-eslint/template/no-autofocus': ['error'], + '@angular-eslint/template/no-distracting-elements': ['error'], + '@angular-eslint/template/role-has-required-aria': ['error'], + '@angular-eslint/template/table-scope': ['error'], + '@angular-eslint/template/valid-aria': ['error'], + }, }, { files: ['**/*.spec.ts'], diff --git a/package-lock.json b/package-lock.json index 57eb6c3f1..6c5de0fa4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -43,6 +43,9 @@ }, "devDependencies": { "@angular-devkit/build-angular": "^19.2.8", + "@angular-eslint/eslint-plugin": "^19.1.0", + "@angular-eslint/eslint-plugin-template": "^19.1.0", + "@angular-eslint/template-parser": "^19.1.0", "@angular/cli": "^19.2.0", "@angular/compiler-cli": "^19.2.0", "@commitlint/cli": "^19.7.1", diff --git a/package.json b/package.json index bda6bf016..b514751e6 100644 --- a/package.json +++ b/package.json @@ -10,12 +10,12 @@ "docs:coverage": "./node_modules/.bin/compodoc -p tsconfig.docs.json --coverageTest 0 --coverageMinimumPerFile 0", "lint": "ng lint", "lint:fix": "ng lint --fix", + "lint:format": "npm run lint:fix && npm run format", "format": "prettier --write .", "format:check": "prettier --check .", - "lint:format": "npm run lint:fix && npm run format", - "prepare": "husky", "ngxs:actions": "ng generate @ngxs/store:actions --name --path", "ngxs:store": "ng generate @ngxs/store:store --name --path", + "prepare": "husky", "start": "ng serve", "start:docker": "ng serve --host 0.0.0.0 --port 4200 --poll 2000", "start:docker:local": "ng serve --host 0.0.0.0 --port 4200 --poll 2000 --configuration local", @@ -63,6 +63,9 @@ }, "devDependencies": { "@angular-devkit/build-angular": "^19.2.8", + "@angular-eslint/eslint-plugin": "^19.1.0", + "@angular-eslint/eslint-plugin-template": "^19.1.0", + "@angular-eslint/template-parser": "^19.1.0", "@angular/cli": "^19.2.0", "@angular/compiler-cli": "^19.2.0", "@commitlint/cli": "^19.7.1", diff --git a/src/app/features/collections/components/collections-discover/collections-discover.component.ts b/src/app/features/collections/components/collections-discover/collections-discover.component.ts index 8851e0fdc..e199ec1f4 100644 --- a/src/app/features/collections/components/collections-discover/collections-discover.component.ts +++ b/src/app/features/collections/components/collections-discover/collections-discover.component.ts @@ -162,7 +162,7 @@ export class CollectionsDiscoverComponent { private getActiveFilters(filters: CollectionsFilters): Record { return Object.entries(filters) - .filter(([key, value]) => value.length) + .filter(([_, value]) => value.length) .reduce( (acc, [key, value]) => { acc[key] = value; diff --git a/src/app/features/project/contributors/components/create-view-link-dialog/create-view-link-dialog.component.ts b/src/app/features/project/contributors/components/create-view-link-dialog/create-view-link-dialog.component.ts index 42cac33fe..d22811fbb 100644 --- a/src/app/features/project/contributors/components/create-view-link-dialog/create-view-link-dialog.component.ts +++ b/src/app/features/project/contributors/components/create-view-link-dialog/create-view-link-dialog.component.ts @@ -53,7 +53,7 @@ export class CreateViewLinkDialogComponent implements OnInit { if (!this.linkName.value) return; const components = (this.config.data?.['sharedComponents'] as ViewOnlyLinkNodeModel[]) || []; - const selectedIds = Object.entries(this.selectedComponents()).filter(([component, checked]) => checked); + const selectedIds = Object.entries(this.selectedComponents()).filter(([_, checked]) => checked); const selected = components .filter((comp: ViewOnlyLinkNodeModel) => diff --git a/src/app/features/registries/components/custom-step/custom-step.component.ts b/src/app/features/registries/components/custom-step/custom-step.component.ts index ed70a2b92..fb1467a82 100644 --- a/src/app/features/registries/components/custom-step/custom-step.component.ts +++ b/src/app/features/registries/components/custom-step/custom-step.component.ts @@ -192,7 +192,7 @@ export class CustomStepComponent implements OnDestroy { [questionKey]: [ ...this.attachedFiles[questionKey].map((f) => { if (f.file_id) { - const { name, ...payload } = f; + const { name: _, ...payload } = f; return payload; } return FilesMapper.toFilePayload(f as OsfFile); @@ -213,7 +213,7 @@ export class CustomStepComponent implements OnDestroy { [questionKey]: [ ...this.attachedFiles[questionKey].map((f) => { if (f.file_id) { - const { name, ...payload } = f; + const { name: _, ...payload } = f; return payload; } return FilesMapper.toFilePayload(f as OsfFile); diff --git a/src/app/features/search/components/filters/institution-filter/institution-filter.component.ts b/src/app/features/search/components/filters/institution-filter/institution-filter.component.ts index 930af8f6d..dd69cdd5b 100644 --- a/src/app/features/search/components/filters/institution-filter/institution-filter.component.ts +++ b/src/app/features/search/components/filters/institution-filter/institution-filter.component.ts @@ -1,10 +1,11 @@ import { Store } from '@ngxs/store'; +import { TranslateModule } from '@ngx-translate/core'; + import { Select, SelectChangeEvent } from 'primeng/select'; import { ChangeDetectionStrategy, Component, computed, effect, inject, signal, untracked } from '@angular/core'; import { FormsModule } from '@angular/forms'; -import { TranslateModule } from '@ngx-translate/core'; import { ResourceFiltersSelectors, SetInstitution } from '../../resource-filters/store'; import { GetAllOptions, ResourceFiltersOptionsSelectors } from '../store'; diff --git a/src/app/shared/components/affiliated-institution-select/affiliated-institution-select.component.ts b/src/app/shared/components/affiliated-institution-select/affiliated-institution-select.component.ts index f3b7f715c..2263bca71 100644 --- a/src/app/shared/components/affiliated-institution-select/affiliated-institution-select.component.ts +++ b/src/app/shared/components/affiliated-institution-select/affiliated-institution-select.component.ts @@ -48,7 +48,6 @@ export class AffiliatedInstitutionSelectComponent implements OnInit { }); effect(() => { - const userInstitutions = this.userInstitutions(); if (this.selectAllByDefault()) { untracked(() => { this.selectAll(); diff --git a/src/app/shared/constants/remove-nullable.const.ts b/src/app/shared/constants/remove-nullable.const.ts index 59872b241..615efe166 100644 --- a/src/app/shared/constants/remove-nullable.const.ts +++ b/src/app/shared/constants/remove-nullable.const.ts @@ -1,6 +1,5 @@ export function removeNullable(obj: T): Partial { return Object.fromEntries( - // eslint-disable-next-line @typescript-eslint/no-unused-vars Object.entries(obj).filter(([_, value]) => value !== null && value !== undefined) ) as Partial; }