Skip to content
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

Fix: ng tests #2742

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Fix: ng tests #2742

wants to merge 20 commits into from

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Jan 22, 2025

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

Provide a number of fixes across various ng components and services

Review Notes

I don't think we need all tests passing before merge, better to not let this get too stale over time as will build up conflicts with ongoing prs and tests. But feel free if wanting to push any extra commits here if wanted. Otherwise further test fixes can be included in a follow-up

Should note that most of the code changes are just to stub/import mock providers and such. There are a couple minor changes to update some mocked services, but as these aren't currently integrated into CI anyway any accidental side-effect test breakages can just be resolved during ongoing work to general fixes.

Dev Notes

See list below of summary of common issues and fixes implemented.

Pipe 'ExamplePipe' not found
declarations: [ExamplePipe]

Can't bind to 'routerLink'

imports: [RouterModule]

ion-content is not a known element

imports: [IonicModule]

tmpl-some-component is not a known element
Any referenced child component importing child component into declarations, e.g.

declarations: [MainComponent, TmplSomeComponent]

If this has significant knock-ons (e.g. child component has lots of dependencies, can also just mock and use that in declaration instead

@Component({ selector: "tmpl-some-component", template: "<div></div>" })
class MockTmplSomeComponent {}

No provider for InjectionToken Application Configuration!
usually side-effect from dependent DeploymentService

{ provide: DeploymentService, useValue: new MockDeploymentService({}) }

No provider for HttpClient!
usually side-effect from dependent AppDataService

{ provide: AppDataService, useValue: new MockAppDataService() }

or if using directly import testing module

imports: [HttpClientTestingModule]

No provider for ModalController!
If dependent of parent service use either mock or stub of parent service.

If required by component/service then stub as required

{ provide: ModalController, useValue: {} },

Cannot read properties of undefined (reading 'parameter_list')
Component init populates data from parameter_list, set in beforeEach hook with mocked row values as required

fixture.componentRef.setInput("row", MOCK_ROW);
await fixture.whenStable();
component = fixture.componentInstance;

Misc Errors
There were a handful of errors related to child dependencies which were being somewhat tricky/problematic. In these cases I've simply marked the default test as skipped for now (using the xit shorthand), so that it can be addressed in the future

xit("should create", () => {
    expect(component).toBeTruthy();
  });

Code Changes

The only frontend code changes that I'm aware of:

  • make firebase config optional in type definitions (although default value {config: null} retained for now to avoid knock-ons)

Git Issues

Progress towards #2196 (but can only close once all tests passing and integrated into CI)
Closes #

Screenshots/Videos

Current status. Should not that number of fails is not truly indicative of number of issues to resolve as many components/services have multiple issues to fix but fail when first encountered (so is a lower bound)

image

@chrismclarke chrismclarke changed the title Fix/ng tests 2 Fix: ng tests Jan 22, 2025
@chrismclarke
Copy link
Member Author

image

Just as a quick update to say most nullInjectorIssues should now be fixed, so moving onto more component-specific issues next. Still good to merge at any point and can just continue with follow-up PRs

@chrismclarke
Copy link
Member Author

Another update, now all passing (except 4 which I've marked as skipped).
I think this should be good for now, individual test suites can be improved over time and when working on specific components. I'll look into adding to CI for next steps

image

@chrismclarke
Copy link
Member Author

@jfmcquade
I've just updated, resolved conflicts with recently merged PRs, and ensured all current tests are passing

image

Given that this will just continue to fall out of sync with any other tests introduced I think it would be good to merge here (as mentioned previously, doesn't need a formal review as the PR is focused just on fixing and unblocking broken test suites)

I'll probably go ahead and just enable admin override to merge (as would be good to have tests passing to compare some pending code changes against), feel free to add a post-merge review when you have the chance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant