test(ivy): add exhaustive inheritance tests #30442
Conversation
benlesh
added
refactoring
benlesh
added
action: merge
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
packages/core/test/acceptance/inherit_definition_feature_spec.ts
Outdated
Show resolved
Hide resolved
|
@pkozlowski-opensource sorry, that link isn't working for me for some reason. What's the review? |
alxhub
added
the
action: review
- Adds inheritance tests for many combinations of directive, component and bare class inheritance - Adds tests that are failing in ivy, but should work, marks them with `xit` and a TODO. - Removes render3 unit tests that cover the same things.
benlesh
added
merge: caretaker note
|
Caretaker: @pkozlowski-opensource's concerns were addressed, and @mhevery is a global approver. |
| } | ||
|
|
||
| TestBed.configureTestingModule({declarations: [AppComp, SubDirective]}); | ||
| TestBed.overrideComponent( |
There was a problem hiding this comment.
nit: would be probably shorter / more obvious to write as TestBed.overrideTemplate(AppComp, '<div subDir [someInput]="1"></div>')?
Applies to this line and all the other places below
There was a problem hiding this comment.
I am actually really against that sort of test writing. Tests should repeat themselves as much as possible, and have everything explicitly and concisely defined within the test block. Dry tests invite mistakes where another developer comes along and changes the shared testing functionality and could potentially break other tests in such a way that they still pass but are no longer valid. I was honestly uneasy even writing the lifecycle hooks inheritance tests the way I did.
Explicit, inline code, while verbose, is by its very nature more readable than code relying on code that exists many lines away and/or relies on underlying magic.
There was a problem hiding this comment.
I gotta say I'm with Pawel on this one. I don't think adding the new Component invocation actually makes things clearer. I think it's confusing and doesn't actually align with how people write tests in the wild.
There was a problem hiding this comment.
Discussed offline: There was some confusion here. :)
The tests above were pre-existing and the diff made it look like I added new ones. I was confused about what was being asked, and everyone else was probably equally confused by my response. Here is a PR that is attempting to address that issue: #30522
| const fixture = TestBed.createComponent(AppComp); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(log).toEqual(['on changes!']); |
There was a problem hiding this comment.
nit: as a sanity check I would have a separate test that asserts that:
- nothing is logged when there is no
ngOnChanges()(otherwise all the tests can be made passing but pushing initial'on changes!'to the mocklogthat you are using; ngOnChanges()gets invoked when input changes.
One such test would be more than enough. Sorry for being pedantic but I like to have those sanity checks in place to make sure that we are not missing any obvious things.
There was a problem hiding this comment.
I didn't write these tests. They were simply moved by reformatting. It's probably worth noting the desired changes, and we can add them in a followup PR.
There was a problem hiding this comment.
Can you please add a TODO for this?
| }); | ||
| }); | ||
|
|
||
| it('should inherit inputs', () => { |
There was a problem hiding this comment.
sounds like it should be a separate describe as we are "switching" from lifecycle hooks to inputs / outputs
There was a problem hiding this comment.
would be great to add more sanity tests for inputs with the same name present in super and sub classes and with different:
- type
- alias
In those cases we've got a clear collision and we should be resolving somehow but I'm not sure what VE / ivy is doing atm. Same applies to outputs.
Could you please add tests for those cases (I'm not sure we need them for all the inheritance cases.
There was a problem hiding this comment.
I agree that we should add more tests. Can we do this in a follow up PR?
There was a problem hiding this comment.
TODOs added in #30522
|
|
||
| it('should compose host bindings (non-style related)', () => { | ||
| class SuperDirective { | ||
| @HostBinding('title') |
There was a problem hiding this comment.
I haven't seen any test for host expressed as host: {...} . Also can't find any tests for "host listener". Could you please add those? Again, I don't think we need those tests for each and every inheritance case.
There was a problem hiding this comment.
I think it sounds important, but I'd like to add these in a follow up PR. Unfortunately, we'd probably want to add them for each inheritance case to be thorough.
|
@benlesh good efforts on tests here. I've left some comments but generally speaking I think that we've got 2 axis to cover here:
From what I can see this PR is "various inheritance combinations" reach but a bit light on "various inherited features". Personally I would prefer to see more comprehensive tests for a single feature (I left notes for cases that were coming to my mind when I was going through the code) and then just sanity checks for various inheritance combinations. Otherwise I I feel like we are repeating same tests over and over again without going too deep into any particular area. I'm saying this since if I recall that @alxhub discovered several places where ivy and VE differ and I don't see this reflected in your tests. Anyway, good tests here, but I think that we can improve by going deeper and scale back on duplication for each various inheritance combinations. WDYT? |
|
@pkozlowski-opensource I agree that there are a lot more tests to add. This PR was just an improvement over the very small number of render3 tests we had, and was already a little out of scope to the point where I was a bit concerned I was taking time away from other important team goals. We should probably catalog the ideas you have here for more tests and circle back to them in a separate PR. We definitely still have some dark spaces. |
There was a problem hiding this comment.
@pkozlowski-opensource I agree that there are a lot more tests to add. This PR was just an improvement over the very small number of render3 tests we had, and was already a little out of scope to the point where I was a bit concerned I was taking time away from other important team goals.
We should probably catalog the ideas you have here for more tests and circle back to them in a separate PR. We definitely still have some dark spaces.
I'm totally cool with adding more tests in a separate PR - I was enumerating all those case while reading code assuming that you are after exhaustive coverage. Also my comments about describe were assuming that you are going to add more tests.
LGTM as this PR clearly improves things but could you please create a JIRA issue with missing tests?
| } | ||
|
|
||
| TestBed.configureTestingModule({declarations: [AppComp, SubDirective]}); | ||
| TestBed.overrideComponent( |
There was a problem hiding this comment.
I gotta say I'm with Pawel on this one. I don't think adding the new Component invocation actually makes things clearer. I think it's confusing and doesn't actually align with how people write tests in the wild.
| const fixture = TestBed.createComponent(AppComp); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(log).toEqual(['on changes!']); |
There was a problem hiding this comment.
Can you please add a TODO for this?
|
Follow up PR is here #30522 |
- Adds inheritance tests for many combinations of directive, component and bare class inheritance - Adds tests that are failing in ivy, but should work, marks them with `xit` and a TODO. - Removes render3 unit tests that cover the same things. PR Close angular#30442
|
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. |
xitand a TODO.NOTE: There is a JIRA ticket filed for the tests marked with
xit: https://angular-team.atlassian.net/browse/FW-1324