-
Notifications
You must be signed in to change notification settings - Fork 95
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
i-bem__dom: Fix leaking test #1369
Conversation
} | ||
} | ||
} | ||
}); | ||
|
||
DOM.decl('block2'); | ||
|
||
DOM.init(BEMHTML.apply({ | ||
var node = $(BEMHTML.apply({ |
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.
в скоупе есть rootNode
, которая потом afterEach
дестрактится
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.
но мы в неё же не можем подмешать другой onSetMod.js.inited?
a473efb
to
d01ccf2
Compare
|
||
delete DOM.blocks['block']; | ||
delete DOM.blocks['block2']; | ||
|
||
res.should.be.eql([true, true]); |
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.
зачем res
? можно проверить прямо в том месте где присваивание — всё работает синхронно
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.
синхронно дестракт не вызовется, потому что throw будет в случае облома.
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.
Хотя, это же тесты, и если оно упало — можно забить на всё остальное, потому что 1 тест это уже плохо.
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.
ок, согласен — тогда оставь так
Just found that accindentally and decide to fix.
Am I right here?
/cc @veged @dfilatov @narqo