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.

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

refactor(ivy): remove unused embedded view instructions #34715

Open
wants to merge 5 commits into
base: master
from

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jan 10, 2020

This commit performs a cleanup and removes unused embedded view instructions and corresponding tests.

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Jan 13, 2020

@AndrewKushnir thnx for starting this cleanup - soooooo many things will be easier when this cleanup lands!

I did the first pass on this PR and left some small comment. More importantly, though, I think that we should migrate more of the tests from the test/render3 folder to TestBed. I've left some comments / suggestions but IMO we should migrate all tests that:

  • are not specifically testing for DOM insertion with JS blocks;
  • don't exist as an acceptance test already.

I know that it will be more effort but would be great not to loose test coverage....

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:rm_unused_embedded_views_instructions branch from d115a63 to eff7351 Jan 18, 2020
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Jan 18, 2020

Hi @pkozlowski-opensource, thanks for the review. I've refactored and moved the tests that you mentioned to the acceptance folder. Could you please let me know if there are some other tests that would benefit from moving to acceptance folder? Thank you.

Copy link
Member

pkozlowski-opensource left a comment

This LGTM overall, just left one comment regarding a test that we might want to migrate. Other tests seem to be either JS-blocks specific or exercising very basic scenarios (which, I would assume, are already covered in other acceptance tests).

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:rm_unused_embedded_views_instructions branch from e99d11f to 83405b9 Jan 23, 2020
@AndrewKushnir AndrewKushnir marked this pull request as ready for review Jan 23, 2020
@AndrewKushnir AndrewKushnir requested review from IgorMinar and angular/framework-global-approvers-for-docs-only-changes as code owners Jan 23, 2020
This commit performs a cleanup and removes unused embedded view instructions and corresponding tests.
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:rm_unused_embedded_views_instructions branch from 83405b9 to 2a3ab77 Jan 25, 2020
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Jan 25, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.