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.

Sign up for GitHub

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

Jump to bottom

Root cause analysis for the remaining projection tests #28152

Closed
wants to merge 2 commits into from
Closed

Root cause analysis for the remaining projection tests #28152

wants to merge 2 commits into from

Conversation

pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jan 15, 2019 •
edited

There are 2 commits in this PR:

  • fix that make sure that we unlink nodes (.next property) when we re-distribute those nodes amongst different projection buckets - this fixes 3 existing tests;
  • a root cause analysis for the one remaining test (see commit message for more details).

@pkozlowski-opensource pkozlowski-opensource requested a review from a team as a code owner January 15, 2019 14:28
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@mary-poppins
Copy link

You can preview 6aae8b2 at https://pr28152-6aae8b2.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource force-pushed the ivy_core_remaining_unknown_projection branch from 6aae8b2 to 392f30a Compare January 15, 2019 14:53
@mary-poppins
Copy link

You can preview 392f30a at https://pr28152-392f30a.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Jan 15, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 15, 2019
@AndrewKushnir
Copy link
Contributor

@pkozlowski-opensource FYI PR #27849 is now merged into master, so you can rebase from the latest master now. Thank you.

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 15, 2019
@ngbot
Copy link

ngbot bot commented Jan 15, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    failure status "cla/google" is failing
    pending missing required labels: cla: yes
    pending forbidden labels detected: cla: no
    pending status "google3" is pending

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.

@mhevery mhevery added cla: yes and removed cla: no labels Jan 15, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@mhevery
Copy link
Contributor

mhevery commented Jan 15, 2019

http://test/OCL:229423476:BASE:229423567:1547585771177:206f48a8

@AndrewKushnir
Copy link
Contributor

@pkozlowski-opensource I put "cleanup" label for now, since this PR has conflicts with master branch (and needs to be rebased, since PR #27849 is merged). Please add the "merge" label back once this PR is rebased. Thank you.

@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 15, 2019
@pkozlowski-opensource pkozlowski-opensource force-pushed the ivy_core_remaining_unknown_projection branch from 392f30a to 1f73f87 Compare January 16, 2019 08:44
@googlebot
Copy link

CLAs look good, thanks!

@mary-poppins
Copy link

You can preview 1f73f87 at https://pr28152-1f73f87.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource force-pushed the ivy_core_remaining_unknown_projection branch from 1f73f87 to aef1ebf Compare January 16, 2019 08:57
@pkozlowski-opensource
test(ivy): add root cause analysis for the remaining projection test
aef1ebf 
Initial thinking was that the bug is in the content projection logic but
it turned out to be a wrong assumption - hence adding a test to illustrate
that basic content projection of view containers works correctly.

What fails in the marked test is the logic quering debug nodes - content
peojection is fine but we never create the 'B' text node since we call
show() method on the "wrong" directive instance.
@mary-poppins
Copy link

You can preview aef1ebf at https://pr28152-aef1ebf.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 16, 2019
@pkozlowski-opensource
Copy link
Member Author

merge-assistance as it re-requires g3 presubmit...

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jan 16, 2019 •
edited

Presubmit #2

@alxhub alxhub closed this in 808898d Jan 17, 2019
alxhub pushed a commit that referenced this pull request Jan 17, 2019
@pkozlowski-opensource @alxhub
test(ivy): add root cause analysis for the remaining projection test (#…
1a85de3 
…28152)

Initial thinking was that the bug is in the content projection logic but
it turned out to be a wrong assumption - hence adding a test to illustrate
that basic content projection of view containers works correctly.

What fails in the marked test is the logic quering debug nodes - content
peojection is fine but we never create the 'B' text node since we call
show() method on the "wrong" directive instance.

PR Close #28152
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@pkozlowski-opensource @ngfelixl
fix(ivy): properly project individual nodes (angular#28152)
e26eeb1 
PR Close angular#28152
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@pkozlowski-opensource @ngfelixl
test(ivy): add root cause analysis for the remaining projection test (a…
4f487a2 
…ngular#28152)

Initial thinking was that the bug is in the content projection logic but
it turned out to be a wrong assumption - hence adding a test to illustrate
that basic content projection of view containers works correctly.

What fails in the marked test is the logic quering debug nodes - content
peojection is fine but we never create the 'B' text node since we call
show() method on the "wrong" directive instance.

PR Close angular#28152
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pkozlowski-opensource @googlebot @mary-poppins @AndrewKushnir @mhevery