Skip to content

Fix the karma suite and run it in CI - #90

Open
matthewberry wants to merge 3 commits into
mainfrom
mjberry/fix-karma-specs
Open

matthewberry wants to merge 3 commits into
mainfrom
mjberry/fix-karma-specs

Conversation

@matthewberry

Copy link
Copy Markdown
Contributor

Summary

On main, the karma suite fails to compile, so ng test never runs a single spec. Once the compile errors are fixed, 26 of 41 specs fail. Nothing in CI runs the tests, which is how it drifted. This PR makes all 41 pass and adds a workflow that runs them.

Changes

  • Compile errors (3):
    • molecule-name-validator and smiles-validator specs called the directive constructors with no arguments, but they now inject services. The specs now pass stubs.
    • entity-ecnumber.component.spec.ts imported EntityEcnumberComponent, but the class is now EntityECNumberComponent.
  • Missing providers (24 specs): most were the default "should create" specs, failing on No provider for HttpClient / ActivatedRoute. Once those were added, the next errors were EnvironmentService and animations. The new src/app/testing/test-providers.ts holds what main.ts provides at bootstrap: provideHttpClient(), provideHttpClientTesting(), provideRouter([]) and provideNoopAnimations(). It also stubs EnvironmentService, because main.ts loads its config in an APP_INITIALIZER that TestBed never runs, and every service that injects it calls getEnvConfig() in its constructor. Each failing spec now has providers: testProviders.
  • Required inputs (4 specs): ExternalLinkComponent, FilterComponent, ReactionSchemeComponent and Molecule3dComponent read their inputs during the first change detection. The specs now set them. ReactionScheme and Molecule3d use setInput so that ngOnChanges fires.
  • FontMatchComponent is standalone, so its spec now imports it instead of declaring it.
  • CI: the new .github/workflows/test.yml runs npm ci and ng test --watch=false --browsers=ChromeHeadless on Node 22, matching the Dockerfile. It triggers the same way as docker.yml: pushes to main/develop and every PR.

Testing

  • Locally, ng test --watch=false --browsers=ChromeHeadless: 41 of 41 passed.
  • Before this PR, on main with only the compile errors fixed: 15 passed, 26 failed.
  • This PR's own run of the new workflow is the first test of the workflow itself.

The run still prints some noise that doesn't fail it:

  • 64 PrimeNG baseZIndex deprecation warnings.
  • The ECNumber page logs "No reaction scheme found for EC number undefined", because the spec has no route param.
  • Two Injector has already been destroyed errors from async work that finishes after a spec tears down. I didn't track down which spec causes them.

Conflicts

This PR and #89 both edit the provider setup in about-page.component.spec.ts. Whichever merges second needs a small rebase. If #89 goes second, its spec can use providers: [...testProviders, { provide: OEDService, useValue: oedService }].

Most of these specs still only check that the component can be created. This PR makes them run; it doesn't add coverage.

The suite hadn't compiled on main, and once it did, 26 of 41 specs failed.
Nothing ran it in CI, so it drifted.

- Fix three compile errors: two directive specs called constructors that now
  take services, and the ECNumber spec used the component's old class name.
- Add src/app/testing/test-providers.ts with what main.ts provides at
  bootstrap (HttpClient, router, animations) plus an EnvironmentService stub,
  and use it in the component specs that need it.
- Set required inputs in four specs, and import FontMatchComponent instead of
  declaring it, since it is standalone.
- Add a Test workflow that runs ng test on pushes to main/develop and on PRs.
Comment thread .github/workflows/test.yml Fixed
npm ci failed with a 401 on @moleculemaker/dev-tool, because the Test
workflow never passed GITHUB_TOKEN to npm the way docker.yml does. Set
the registry and token through setup-node, and limit the token to
contents: read and packages: read, which also answers the CodeQL
missing-permissions alert.
testProviders already provides the router through provideRouter([]), so
the spec was setting it up twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants