Skip to content
This repository was archived by the owner on Jul 29, 2024. It is now read-only.

feat(plugins): add postTest hook for plugins#1859

Merged
juliemr merged 1 commit intoangular:masterfrom
juliemr:postTest
Feb 26, 2015
Merged

feat(plugins): add postTest hook for plugins#1859
juliemr merged 1 commit intoangular:masterfrom
juliemr:postTest

Conversation

@juliemr
Copy link
Member

@juliemr juliemr commented Feb 26, 2015

Additionally, add some tests to make sure that plugins can fail properly.

Closes #1842

@juliemr
Copy link
Member Author

juliemr commented Feb 26, 2015

@sjelin can you take a look?

lib/plugins.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Thee word "test" seems ambiguous. Is this after each it(), assert(), or describe()?

Copy link
Member Author

Choose a reason for hiding this comment

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

each it() (in Jasmine) - another option would be spec. Preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please update the comment on line 17

Copy link
Contributor

Choose a reason for hiding this comment

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

We already use spec to refer to the whole file. testcase might make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated comments.

@juliemr
Copy link
Member Author

juliemr commented Feb 26, 2015

Comments addressed.

lib/plugins.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is consistent with what you put in plugins/README.md. Shouldn't it be (pluginObj[funName] || noop).apply(this, [pluginConf].concat([].slice.call(arguments)))?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe it would be good to test this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, and added tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both blocks of this conditional are identical

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, one says 'passing' and one says 'failing'

Copy link
Member Author

Choose a reason for hiding this comment

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

Modified to duplicate fewer lines.

Additionally, add some tests to make sure that plugins can fail properly.

Closes angular#1842
@sjelin
Copy link
Contributor

sjelin commented Feb 26, 2015

LGTM

@juliemr juliemr merged commit 658902b into angular:master Feb 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugins: Add ability to hook in at the end of each test

3 participants