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

feat($exceptionHandlerProvider): add mode `logAndRethrow` #10564

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@DavidSouther
Contributor

DavidSouther commented Dec 23, 2014

Mode logAndRethrow captures the logged exception for later inspection by test
assertions, while also throwing the exception for when the implementation
depends on thrown behavior (eg leaving the method early).

Depends on #10563
Closes #10540

@googlebot googlebot added the cla: yes label Dec 23, 2014

fix($exceptionHandlerProvider): call `inject()` to run tests
In the current angular-mocksSpec, the tests for $exceptionHandlerProvider
call `module` to run tests on `$exceptionHandlerProvider.mode()`, but do
not call `inject()` to pump the module definitions.
@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 23, 2014

Member

I think I agree that we should just log inside rethrow, since it won't do any harm and adding yet another config level seems unattractive and unnecessary.

Member

petebacondarwin commented Dec 23, 2014

I think I agree that we should just log inside rethrow, since it won't do any harm and adding yet another config level seems unattractive and unnecessary.

@DavidSouther

This comment has been minimized.

Show comment
Hide comment
@DavidSouther

DavidSouther Dec 23, 2014

Contributor

I think I agree

:) I'll redo it that way!

Contributor

DavidSouther commented Dec 23, 2014

I think I agree

:) I'll redo it that way!

feat($exceptionHandlerProvider): Log errors when rethrowing.
Current behavior draws a distinction between `log` and `rethrow` modes of
$exceptionHandler in ngMocks. This unifies the behaviors, with both modes
logging the errors while the `log` mode does not throw.

Closes #10540
* - `log`: Sometimes it is desirable to test that an error is thrown, for this case the `log`
* mode stores an array of errors in `$exceptionHandler.errors`, to allow later
* assertion of them. See {@link ngMock.$log#assertEmpty assertEmpty()} and
* {@link ngMock.$log#reset reset()}
* - `rethrow`: If any errors are passed to the handler in tests, it typically means that there
* is a bug in the application or test, so this mock will make these tests fail.
* For any impementations that expect exceptions to be thrown, the `rethrow` mode

This comment has been minimized.

@petebacondarwin
@petebacondarwin
@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 23, 2014

Member

@DavidSouther - thanks for this.
This looks good. I will fix the typo when merging.
The commit message needs just a tiny bit of tweaking:

  • No uppercase letter at the start of the description
  • No full stop at the end of the first line

which I will also fix when merging

Member

petebacondarwin commented Dec 23, 2014

@DavidSouther - thanks for this.
This looks good. I will fix the typo when merging.
The commit message needs just a tiny bit of tweaking:

  • No uppercase letter at the start of the description
  • No full stop at the end of the first line

which I will also fix when merging

petebacondarwin added a commit that referenced this pull request Dec 23, 2014

feat(ngMock/$exceptionHandler): log errors when rethrowing
Now the `rethrow` mode will also record a log of the error in the same
way as the `log` mode.

Closes #10540
Closes #10564

petebacondarwin added a commit that referenced this pull request Dec 23, 2014

feat(ngMock/$exceptionHandler): log errors when rethrowing
Now the `rethrow` mode will also record a log of the error in the same
way as the `log` mode.

Closes #10540
Closes #10564

Conflicts:
	src/ngMock/angular-mocks.js
@DavidSouther

This comment has been minimized.

Show comment
Hide comment
@DavidSouther

DavidSouther Dec 23, 2014

Contributor

@petebacondarwin The branch is closed - did you want me to fix the commit message, typo, and ask for reopen?

Contributor

DavidSouther commented Dec 23, 2014

@petebacondarwin The branch is closed - did you want me to fix the commit message, typo, and ask for reopen?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 23, 2014

Member

I merged it into 1.3.x and 1.4.x already

Member

petebacondarwin commented Dec 23, 2014

I merged it into 1.3.x and 1.4.x already

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 23, 2014

Member

I closed the PR by mentioning it in the commit that landed in master

Member

petebacondarwin commented Dec 23, 2014

I closed the PR by mentioning it in the commit that landed in master

@DavidSouther

This comment has been minimized.

Show comment
Hide comment
@DavidSouther

DavidSouther Dec 23, 2014

Contributor

Gotcha! I see where the merge happened; I'm just so used to seeing the
purple Github "merged" icon :) Thanks for your help, and thanks for taking
this change!

On Tue Dec 23 2014 at 2:25:56 PM Pete Bacon Darwin notifications@github.com
wrote:

I closed the PR by mentioning it in the commit that landed in master


Reply to this email directly or view it on GitHub
#10564 (comment).

Contributor

DavidSouther commented Dec 23, 2014

Gotcha! I see where the merge happened; I'm just so used to seeing the
purple Github "merged" icon :) Thanks for your help, and thanks for taking
this change!

On Tue Dec 23 2014 at 2:25:56 PM Pete Bacon Darwin notifications@github.com
wrote:

I closed the PR by mentioning it in the commit that landed in master


Reply to this email directly or view it on GitHub
#10564 (comment).

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 24, 2014

Member

Yes, in Angular we don't use GitHub's merge as we rather have a linear git history.
We always rebase and do fast forward merges only (and cherry pick to other version branches).
By adding the "Closes #XXX" phrase to the bottom of the commit we can trigger Github to automatically close the PR. Perhaps we should ask them to support a "Merges #XXX" tag too?

Member

petebacondarwin commented Dec 24, 2014

Yes, in Angular we don't use GitHub's merge as we rather have a linear git history.
We always rebase and do fast forward merges only (and cherry pick to other version branches).
By adding the "Closes #XXX" phrase to the bottom of the commit we can trigger Github to automatically close the PR. Perhaps we should ask them to support a "Merges #XXX" tag too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment