馃彈馃悰 Add a more robust mechanism for dynamically skipping tests #17265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17265 +/- ##
==========================================
- Coverage 77.73% 77.73% -0.01%
==========================================
Files 564 564
Lines 41217 41217
==========================================
- Hits 32041 32040 -1
- Misses 9176 9177 +1
Continue to review full report at Codecov.
|
test/_init_tests.js
Outdated
| if (this._runnable.title != '"before all" hook') { | ||
| throw new Error('skipTest() can only be called from within before()'); | ||
| } | ||
| this.test.parent.pending = true; |
There was a problem hiding this comment.
Workaround for mochajs/mocha#2683. Comment added.
test/functional/test-fixed-layer.js
Outdated
| doc.body.appendChild(container); | ||
| }); | ||
| // Can only test when SD is supported. | ||
| describe.configure().ifChrome('shadow transfer', function() { |
There was a problem hiding this comment.
This is Safari, too. You can use generic if:
describe.configure().if(() => Element.prototype.attachShadow)| @@ -438,293 +438,292 @@ describes.realWin('ViewportBindingIosEmbedWrapper', {ampCss: true}, env => { | |||
| }); | |||
| }); | |||
|
|
|||
| describes.realWin('ViewportBindingIosEmbedShadowRoot_', {ampCss: true}, env => { | |||
| describes.realWin('ViewportBindingIos', {ampCss: true}, env => { | |||
There was a problem hiding this comment.
Please use the old name. We have multiple iOS bindings.
| return whenDocumentReady(win.document).then(() => { | ||
| expect(root).to.have.class('i-amphtml-ios-overscroll'); | ||
| // Can only test when SD is supported. | ||
| describe.configure().ifChrome('EmbedShadowRoot_', function() { |
There was a problem hiding this comment.
Safari also has attachShadow. Also, it's a Safari specific test suite. 馃槈
|
@jridgewell PTAL. |
| expect(root).to.have.class('i-amphtml-ios-overscroll'); | ||
| // Can only test when SD is supported. | ||
| describe.configure().if(() => Element.prototype.attachShadow).run('Viewport' + | ||
| 'BindingIosEmbedShadowRoot_', function() { |
test/functional/test-fixed-layer.js
Outdated
| doc.body.appendChild(container); | ||
| }); | ||
| // Can only test when SD is supported. | ||
| describe.configure().if(() => Element.prototype.attachShadow).run('shadow ' + |
There was a problem hiding this comment.
Note that this now checks whether shadow DOM is supported in the test runner window rather than the "realWin".
There was a problem hiding this comment.
Given that this is a nested describe within a describes.realWin, will it not have the same effect? (From actually running the test, It seemed to me that we do have the desired skipping behavior.)
There was a problem hiding this comment.
Most likely fine unless we want to do something weird in describes later.
| 'this\\.skip\\(\\)': { | ||
| message: 'Use of `this.skip()` is forbidden. Use `this.skipTest()` ' + | ||
| 'from within a `before()` block instead. See #17245.', | ||
| checkInTestFolder: true, |
There was a problem hiding this comment.
Does this only match tests or also source files?
There was a problem hiding this comment.
It matches all files, but there are no instances of this.skip() in source files. I've updated the error message to be more clear.
There are two issues with mocha's
this.skip():beforeEach()block orit()block will causeafterEach()to be skipped (this.skip() is also skipping teardown()/afterEach()聽mochajs/mocha#2148)describeblock (skip doesn't work for nested describes聽mochajs/mocha#2683)According to https://mochajs.org/#inclusive-tests, the recommended place in which to dynamically skip tests is within a
before()block.This PR does the following:
this.skipTest(), a more robust version ofthis.skip()that...before()blockdescribeblocksthis.skip()within tests, and recommends the use ofthis.skipTest()insteadWith this, dirty global state due to inadvertent skipping of
afterEach()should no longer be a problem.Fixes #17245
Partial fix for #14360