-
Notifications
You must be signed in to change notification settings - Fork 2k
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
ui: persist node drain settings #11368
Conversation
Ember Asset Size actionAs of 0eaa94a Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
Ember Test Audit detected these flaky tests on b58f27a:
|
} | ||
|
||
module('Acceptance | client detail', function(hooks) { | ||
setupApplicationTest(hooks); | ||
setupMirage(hooks); | ||
|
||
hooks.beforeEach(function() { | ||
window.localStorage.clear(); |
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.
This is a learning question. Why wouldn't we put this into a clean up hook?
hooks.afterEach(function() {
window.localStorage.clear();
}
Isn't that more in line with what we're trying to do?
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.
It's mostly just two different perspectives. One says "make sure we have a clean environment before running the test", the other says "make sure we clean-up after ourselves before we leave".
The only difference is on the edges of the test cases. with an afterEach
, the first test may run with in a dirty environment, and with beforeEach
the last test case may leave some side effects behind.
To avoid these problems it's important to keep tests consistent, and all other tests modules that use localStorage
clears it with beforeEach
.
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.
LGTM only 2 small non-blocking comments.
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on 0eaa94a:
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Persist drain settings in local storage so users don't have to set them again while draining multiple nodes.
Closes #11123