Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

test(router): Add tests to document expected behavior #43449

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
Closed
wants to merge 1 commit into from
Closed

test(router): Add tests to document expected behavior #43449

wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Sep 14, 2021

PR #43102 broke some use-cases. These tests document expected behavior
and would have prevented #43446 and #43447. Recent changes have already
addressed these issues, but it would still be a good idea to cover these
use-cases in tests as well.

@atscott atscott added area: router target: patch This PR is targeted for the next patch release labels Sep 14, 2021
@ngbot ngbot bot added this to the Backlog milestone Sep 14, 2021
@google-cla google-cla bot added the cla: yes label Sep 14, 2021
@ngbot ngbot bot added this to the Backlog milestone Sep 14, 2021
AndrewKushnir
AndrewKushnir approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

packages/router/test/regression_integration.spec.ts Outdated Show resolved Hide resolved
@atscott
test(router): Add tests to document expected behavior
2169362 
PR angular#43102 broke some use-cases. These tests document expected behavior
and would have prevented angular#43446 and angular#43447. Recent changes have already
addressed these issues, but it would still be a good idea to cover these
use-cases in tests as well.
@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Sep 14, 2021
@ngbot
Copy link

ngbot bot commented Sep 14, 2021

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/circleci: lint" is failing
    pending status "ci/circleci: components-repo-unit-tests" is pending
    pending status "ci/circleci: test_aio" is pending
    pending status "ci/circleci: test_aio_local" is pending
    pending status "ci/circleci: test_aio_preview" is pending
    pending status "ci/circleci: test_docs_examples" is pending
    pending status "ci/circleci: test_win" 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.

@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 14, 2021
@AndrewKushnir
Copy link
Contributor

@atscott there was a merge conflict with the patch branch (12.2.x), so I merged this change to master only. Please create a patch-only version of this PR if needed. Thank you.

atscott added a commit to atscott/angular that referenced this pull request Sep 14, 2021
@atscott
test(router): Add tests to document expected behavior
a706f01 
This is a patch port of the tests from angular#43449
@atscott atscott mentioned this pull request Sep 14, 2021
Closed
@bullibasti
Copy link

Help german

AndrewKushnir pushed a commit that referenced this pull request Sep 15, 2021
@atscott @AndrewKushnir
test(router): Add tests to document expected behavior (#43450)
afeaf1e 
This is a patch port of the tests from #43449

PR Close #43450
@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 Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
3 participants
@atscott @AndrewKushnir @bullibasti
Morty Proxy This is a proxified and sanitized view of the page, visit original site.