chore(select): organize tests to better scope compilation #8342
Conversation
googlebot
added
the
cla: yes
jelbourn
changed the title
src/lib/select/select.spec.ts
Outdated
| const commonProviders = [ | ||
| { | ||
| provide: OverlayContainer, useFactory: () => { | ||
| overlayContainerElement = document.createElement('div') as HTMLElement; |
There was a problem hiding this comment.
I might have it in another PR somewhere, but we shouldn't be creating thing element in the first place, we should just inject the OverlayContainer.
There was a problem hiding this comment.
I'd rather address that in a different PR because I don't want to actually change any logic in this one
| * overall test time. | ||
| * @param declarations Components to declare for this block | ||
| */ | ||
| function configureMatSelectTestingModule(declarations) { |
There was a problem hiding this comment.
The declarations could at least be any[] to give it a little bit of type safety.
There was a problem hiding this comment.
I believe TypeScript infers the type here from how the argument is used
src/lib/select/select.spec.ts
Outdated
| it('should clear the selection when the control is reset', fakeAsync(() => { | ||
| fixture.componentInstance.control.setValue('pizza-1'); | ||
| fixture.detectChanges(); | ||
| describe('aria-owns', () => { |
There was a problem hiding this comment.
Is this one supposed to be empty?
There was a problem hiding this comment.
Fixed- I copied the contents to a different describe
There was a problem hiding this comment.
It seems like the empty describe is still there.
src/lib/select/select.spec.ts
Outdated
| trigger.click(); | ||
| fixture.detectChanges(); | ||
| expect(optionInstances[0].selected) | ||
| .toBe(false, 'Expected first option to no longer be |
There was a problem hiding this comment.
Since it's a single test it doesn't need the beforeEach, it could be moved outside or into the "special cases".
src/lib/select/select.spec.ts
Outdated
| trigger.click(); | ||
| fixture.detectChanges(); | ||
| expect(optionInstances[0].selected) | ||
| .toBe(false, 'Expected first option to no longer be |
There was a problem hiding this comment.
We don't need a beforeEach for a single test suite. I doubt that we'll add any more tests to it.
src/lib/select/select.spec.ts
Outdated
|
|
||
| return {getContainerElement: () => overlayContainerElement}; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The indentation here and the ScrollDispatcher override is a little weird. The lambas need to be indented.
src/lib/select/select.spec.ts
Outdated
| it('should clear the selection when the control is reset', fakeAsync(() => { | ||
| fixture.componentInstance.control.setValue('pizza-1'); | ||
| fixture.detectChanges(); | ||
| describe('aria-owns', () => { |
There was a problem hiding this comment.
It seems like the empty describe is still there.
jelbourn
force-pushed
the
select-test-split
branch
from
318e10f to
d5b8d8a
Compare
|
Done; I hadn't exported my changes when I made the comment |
jelbourn
added
action: merge
jelbourn
force-pushed
the
select-test-split
branch
from
d5b8d8a to
e9764a9
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
None of the tests are changed, just reorganized. I tweaked a few test names / assertions to fit on one line.