-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Issue/2819 - Fix / Clarify this.skip behavior in before hooks #3225
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6dec533
Fix Issue 2819 - this.skip should skip tests in nested suites.
bannmoore f966923
fixup! Fix Issue 2819 - this.skip should skip tests in nested suites.
bannmoore d530ef1
docs: Add more details regarding suite-level skip.
bannmoore 45afb25
test: Update error messages in before hook integration tests.
bannmoore 5055a33
test: Remove timeout delays from pending tests.
bannmoore b858b43
test: Consistently throw errors in pending test fixtures.
bannmoore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
test/integration/fixtures/pending/skip-async-before-hooks.fixture.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
'use strict'; | ||
|
||
describe('outer suite', function () { | ||
|
||
before(function () { | ||
console.log('outer before'); | ||
}); | ||
|
||
it('should run this test', function () { }); | ||
|
||
describe('inner suite', function () { | ||
|
||
before(function (done) { | ||
console.log('inner before'); | ||
var self = this; | ||
setTimeout(function () { | ||
self.skip(); | ||
done(); | ||
}, 0); | ||
}); | ||
|
||
beforeEach(function () { | ||
throw new Error('beforeEach should not run'); | ||
}); | ||
|
||
afterEach(function () { | ||
throw new Error('afterEach should not run'); | ||
}); | ||
|
||
it('should not run this test', function () { | ||
throw new Error('inner suite test should not run'); | ||
}); | ||
|
||
after(function () { | ||
console.log('inner after'); | ||
}); | ||
|
||
describe('skipped suite', function () { | ||
before(function () { | ||
console.log('skipped before'); | ||
}); | ||
|
||
it('should not run this test', function () { | ||
throw new Error('skipped suite test should not run'); | ||
}); | ||
|
||
after(function () { | ||
console.log('skipped after'); | ||
}); | ||
}); | ||
|
||
}); | ||
|
||
it('should run this test', function () { }); | ||
|
||
after(function () { | ||
console.log('outer after'); | ||
}); | ||
|
||
}); |
41 changes: 41 additions & 0 deletions
41
test/integration/fixtures/pending/skip-async-before-nested.fixture.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
describe('skip in before with nested describes', function () { | ||
before(function (done) { | ||
var self = this; | ||
setTimeout(function () { | ||
self.skip(); | ||
done(); | ||
}, 0); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
|
||
describe('nested describe', function () { | ||
before(function () { | ||
throw new Error('first level before should not run'); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
|
||
after(function () { | ||
throw new Error('first level after should not run'); | ||
}); | ||
|
||
describe('nested again', function () { | ||
before(function () { | ||
throw new Error('second level before should not run'); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
|
||
after(function () { | ||
throw new Error('second level after should not run'); | ||
}); | ||
}); | ||
}); | ||
}); |
29 changes: 17 additions & 12 deletions
29
test/integration/fixtures/pending/skip-async-before.fixture.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,23 @@ | ||
'use strict'; | ||
|
||
describe('skip in before', function () { | ||
before(function (done) { | ||
var self = this; | ||
setTimeout(function () { | ||
self.skip(); | ||
}, 50); | ||
}); | ||
describe('outer describe', function () { | ||
it('should run this test', function () {}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never thrown'); | ||
}); | ||
describe('skip in before', function () { | ||
before(function (done) { | ||
var self = this; | ||
setTimeout(function () { | ||
self.skip(); | ||
}, 0); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never thrown'); | ||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
}); | ||
|
||
it('should run this test', function () {}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,8 @@ describe('skip in test', function () { | |
var self = this; | ||
setTimeout(function () { | ||
self.skip(); | ||
}, 50); | ||
}, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just remove the value. If omitted, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and I don't see any harm in being explicit, especially in tests. |
||
}); | ||
|
||
it('should run other tests in the suite', function () { | ||
// Do nothing | ||
}); | ||
it('should run other tests in the suite', function () {}); | ||
}); |
57 changes: 57 additions & 0 deletions
57
test/integration/fixtures/pending/skip-sync-before-hooks.fixture.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
'use strict'; | ||
|
||
describe('outer suite', function () { | ||
|
||
before(function () { | ||
console.log('outer before'); | ||
}); | ||
|
||
it('should run this test', function () { }); | ||
|
||
describe('inner suite', function () { | ||
before(function () { | ||
this.skip(); | ||
}); | ||
|
||
before(function () { | ||
console.log('inner before'); | ||
}); | ||
|
||
beforeEach(function () { | ||
throw new Error('beforeEach should not run'); | ||
}); | ||
|
||
afterEach(function () { | ||
throw new Error('afterEach should not run'); | ||
}); | ||
|
||
after(function () { | ||
console.log('inner after'); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('inner suite test should not run'); | ||
}); | ||
|
||
describe('skipped suite', function () { | ||
before(function () { | ||
console.log('skipped before'); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('skipped suite test should not run'); | ||
}); | ||
|
||
after(function () { | ||
console.log('skipped after'); | ||
}); | ||
}); | ||
}); | ||
|
||
it('should run this test', function () { }); | ||
|
||
after(function () { | ||
console.log('outer after'); | ||
}) | ||
|
||
}); |
39 changes: 39 additions & 0 deletions
39
test/integration/fixtures/pending/skip-sync-before-nested.fixture.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
'use strict'; | ||
|
||
describe('skip in before with nested describes', function () { | ||
before(function () { | ||
this.skip(); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
|
||
describe('nested describe', function () { | ||
before(function () { | ||
throw new Error('first level before should not run'); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
|
||
after(function () { | ||
throw new Error('first level after should not run'); | ||
}); | ||
|
||
describe('nested again', function () { | ||
before(function () { | ||
throw new Error('second level before should not run'); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
|
||
after(function () { | ||
throw new Error('second level after should not run'); | ||
}); | ||
}); | ||
}); | ||
}); |
23 changes: 14 additions & 9 deletions
23
test/integration/fixtures/pending/skip-sync-before.fixture.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,20 @@ | ||
'use strict'; | ||
|
||
describe('skip in before', function () { | ||
before(function () { | ||
this.skip(); | ||
}); | ||
describe('outer describe', function () { | ||
it('should run this test', function () {}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never thrown'); | ||
}); | ||
describe('skip in before', function () { | ||
before(function () { | ||
this.skip(); | ||
}); | ||
|
||
it('should never run this test', function () { | ||
throw new Error('never thrown'); | ||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
it('should never run this test', function () { | ||
throw new Error('never run this test'); | ||
}); | ||
}); | ||
|
||
it('should run this test', function () {}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the necessity of doing this asynchronously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boneskull I'm not sure what the "real" use case would look like, but this test validates that an asynchronous skip works with a synchronous nested
describe
. It seemed like something worth checking, even as an edge case. 🤷♀️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of thesesetTimeout()
-based tests are simply race conditions that happen to have worked in @bannmoore's favor. Shouldn't we just disallow use of asyncthis.skip()
? Otherwise, the results are nondeterministic...Nvm. I should have noticed the
done()
callback, but missed it initially. I doubt testing the standard Mocha async callback more than once buys us much, but okay. GivensetTimeout(func, delay)
, setdelay=0
. No point in making the tests any slower than necessary.