RouterTestingHarness should trigger change detection after any navigation

[yjaaidi]

Which @angular/* package(s) are relevant/related to the feature request?

router

Description

TL;DR: When using RouterTestingHarness, navigations triggered using RouterTestingHarness are triggering change detection but subsequent navigations (i.e. triggered through routerLink clicks or Router.navigate() etc...) are not. Developers must then explicitly trigger change detection which can be tricky (e.g. some guard might add some delay etc...). This makes tests more fragile and complex and kind of defeats the purpose of RouterTestingHarness.

More precisely, while RouterTestingHarness triggers change detection when explicitly calling navigateByUrl() or implicitly by passing an initial URL to create()...

angular/packages/router/testing/src/router_testing_harness.ts

Lines 135 to 138 in dedac8d

	afterNextNavigation(TestBed.inject(Router), resolveFn);
	await router.navigateByUrl(url);
	await redirectTrackingPromise;
	this.fixture.detectChanges();

... it doesn't trigger it when navigation is triggered otherwise.

Thanks to @tomalaforge for raising questions on testing + routing in Cypress.
Thanks to @atscott for the quick feedback and help to pinpoint this.

Proposed solution

RouterTestingHarness should trigger change detection after each navigation.
This can be done by subscribing to Router.events and triggering change detection on each NavigationEnd.

@Injectable({providedIn: 'root'})
export class RootFixtureService {
  // ...

  private subscription?: Subscription;

  createHarness(): RouterTestingHarness {
    if (this.harness) {
      throw new Error('Only one harness should be created per test.');
    }
    this.harness = new RouterTestingHarness(this.getRootFixture());

    this.subscription = TestBed.inject(Router).events
      .pipe(filter(e => e instanceof NavigationEnd))
      .subscribe(() => this.harness.detectChanges()); 

    return this.harness;
  }
  
  ngOnDestroy() {
    this.subscription?.unsubscribe();
  }
  
  // ...
}

I'd be happy to submit a PR.

Alternatives considered

Meanwhile, here is a temporary workaround one can use.

async function createHarness(initialUrl: string) {
  const harness = await RouterTestingHarness.create(initialUrl);
  autoDetectChanges(harness);
  return harness;
}

function autoDetectChanges(harness: RouterTestingHarness) {
  TestBed.inject(Router)
    .events.pipe(filter((e) => e instanceof NavigationEnd))
    .subscribe(() => harness.detectChanges());
}

[atscott]

I do think this is a good idea in general. Truthfully, it's not limited to the Router or RouterTestingHarness, but more of a general problem with TestBed and its integration with change detection and NgZone.

TestBed has code that ensures components are created inside the NgZone (see here). This isn't always super helpful because most of the time, just about everything you do in tests is done outside the zone (triggering events on debug elements, directly calling router.navigate, etc)

When we're looking outward to future versions of Angular where NgZone is more optional, we'll have to look at TestBed again and figure out how to give it a different automatic change detection scheduler (currently it's powered by NgZone with autoDetectChanges).

It's likely this issue will be solved or at least be looked at again when we get to NoopNgZone by default. With that, I think it might be premature to build a feature specifically around this problem (at least for now) rather than the root cause.

[angular-robot]

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

[yjaaidi]

OMG! Time flies and I feel like I opened this issue yesterday.

I agree with you @atscott! The root cause is relying on zone.js in a context where most actions are triggered outside of zone...

... but the router test harness is different for the following reasons:

• the change detection problem with other actions (event dispatch etc...) is partially solved because developers are already used to it, or it's wrapped in libraries like Angular testing library
• the router test harness is a higher level testing API so it "might feel" like it should handle the CD magic
• the router test harness makes tests "wider" and if it doesn't handle CD, it will be really hard for developers to handle it compared to other actions. (E.g. if a guard is added with some delay on a routing config under test, this will break tests, and the fix is tricky: hard coded delay or the workaround described above)

That's why I think it's worth a fix.

[angular-robot]

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

[yjaaidi]

TestBed has code that ensures components are created inside the NgZone. This isn't always super helpful because most of the time, just about everything you do in tests is done outside the zone (triggering events on debug elements, directly calling router.navigate, etc)

Haha! Actually, I just opened an issue covering that exact spot #50079

Do you mind if I give it a shot with a PR?
I think that however Angular evolves, existing Zone-based apps using the RouterTestingHarness will have to implement a workaround similar to the one described above. What do you think?

[angular-robot]

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

[atscott]

TL;DR Yes, navigations in tests should automatically trigger change detection. But this should be through the same scheduling mechanism used in the application, not the RouterTestingHarness forcing detectChanges.

After working on the first bits of zoneless Angular, I'm going to close this since I don't believe it should work that way at all. The RouterOutlet calls markForCheck, which will be enough to schedule change detection on its own. Test patterns should/will await thing() when a change detection is scheduled. Causing synchronous change detection by doing a manual detectChanges in tests would mean that the test operates in a way that differs from the application. This is undesirable and at-odds with the whole point of testing.