tests: fix/improve usage of "pending" #11184
Conversation
| @@ -182,7 +182,7 @@ describe('server -> client', function() | |||
| describe('recursive (child) nvim client', function() | |||
| if helpers.isCI('travis') and helpers.is_os('mac') then | |||
| -- XXX: Hangs Travis macOS since e9061117a5b8f195c3f26a5cb94e18ddd7752d86. | |||
| pending("[Hangs on Travis macOS. #5002]", function() end) | |||
| pending("[Hangs on Travis macOS. #5002]") | |||
There was a problem hiding this comment.
unless something changed in busted, this won't work. The function argument is needed.
Same for the other cases.
There was a problem hiding this comment.
There are two cases for pending: 1) when using it instead of it, and 2) when using it inside of a test, where it will stop the test there then also.
There was a problem hiding this comment.
Oh.. this is actually wrong, yes.
But "only" in that it would not display it as skipped then.
I think it is better to replace it with pending in those cases, so that the number of tests is the same, and there is no special no-op test that gets skipped, but is not available in general.
Also e.g. test filters etc would not find the existing (skipped) ones then.
Doesn't seem like the problem is fixed. For example, the build log of the travis macOS build on master shows |
| local describe = describe | ||
| clear() | ||
| if missing_provider('node') then | ||
| describe = function(desc, ...) pending(desc.." (Missing nodejs host, or nodejs version is too old)", ...) end |
There was a problem hiding this comment.
why is this better than the old code? local describe = describe is awkward, and in general the new pattern is less obvious/clear.
There was a problem hiding this comment.
I think it is better since it keeps the number of tests etc (see also previous comment)
(for reference lunarmodules/busted#610)
| before_each(function() | ||
| clear() | ||
| command('python import vim') | ||
| end) | ||
| end |
There was a problem hiding this comment.
why is this if-else better than the old return approach?
There was a problem hiding this comment.
Same reason as before: to register the tests really.
The if/else is needed here to not use the before_each with the test that is only done when the provider is missing.
Use helpers.pending_it
| put ='case3: failed' | ||
| else | ||
| close | ||
| put ='case3: ok' |
There was a problem hiding this comment.
? The whitespace is part of the test.
There was a problem hiding this comment.
Did not fail though with it being removed. But have not really paid attention / looked at the test.
|
This doesn't seem worth the cost.
|
|
Yeah, it's quite messy indeed. Created #11247 for clear misuse. |
No description provided.