aglinxinyuan commented on code in PR #7408:
URL: https://github.com/apache/texera/pull/7408#discussion_r3739643433
##########
frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts:
##########
@@ -18,6 +18,11 @@
*/
import { ComponentFixture, TestBed } from "@angular/core/testing";
+import { RouterTestingModule } from "@angular/router/testing";
+import { By } from "@angular/platform-browser";
+import { HttpClientTestingModule } from "@angular/common/http/testing";
Review Comment:
Removed in 0054c45e52 — though the reasoning needs one correction, since the
stated premise doesn't hold.
`HttpClientTestingModule` was still in the TestBed imports (line 142), so
the top-level import wasn't dead. The conclusion is right anyway for a
different reason: it is no longer *needed*. It was added while chasing the
`texera-user-avatar` dependency chain, and once `StubUserService` cut that
chain at `UserService`, nothing in the suite makes an HTTP call. Verified by
removing it and re-running — 17/17 still pass — so both the import and the
TestBed entry are gone.
##########
frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts:
##########
@@ -122,3 +127,104 @@ describe("BrowseSectionComponent", () => {
});
});
});
+/**
+ * The cards themselves are template-only: the specs above assert the route
map and the cover-URL
+ * cache, but nothing had ever rendered a card, so the per-entity bindings and
their fallbacks were
+ * unpinned. RouterTestingModule supplies the Router that the cards'
routerLink needs.
+ */
+describe("BrowseSectionComponent rendering", () => {
+ let component: BrowseSectionComponent;
+ let fixture: ComponentFixture<BrowseSectionComponent>;
+
+ beforeEach(() => {
+ TestBed.resetTestingModule();
+ TestBed.configureTestingModule({
+ imports: [BrowseSectionComponent, RouterTestingModule.withRoutes([]),
HttpClientTestingModule],
+ providers: [
+ // The cards embed texera-user-avatar, which injects UserService; the
real one drags in
+ // AuthService and its whole dependency chain, so the shared stub
stands in for it.
+ { provide: UserService, useClass: StubUserService },
+ { provide: WorkflowPersistService, useValue: {} },
+ { provide: DatasetService, useValue: {} },
+ { provide: ChangeDetectorRef, useValue: {} },
+ ...commonTestProviders,
Review Comment:
Good catch, and fixed in 0054c45e52 — removed from the new suite and from
the pre-existing one, since it was inert in both.
Worth spelling out why it was inert rather than merely unnecessary: a
component's own `ChangeDetectorRef` is resolved from the node injector, not the
TestBed module injector, so `{ provide: ChangeDetectorRef, useValue: {} }`
never reaches the component regardless. It couldn't have masked a `cdr` call —
but it did suggest to a reader that the component's change detector was
stubbed, which is the more expensive kind of wrong.
Confirmed by removing it and re-running: 17/17 pass either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
