-
Notifications
You must be signed in to change notification settings - Fork 6k
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
mgr/dashboard: Simplify OSD disabled action test #24824
Conversation
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.
I personally do see the need to change the test as I think it was already readable, but that may be due to the fact that I am the author of that code. I'm excited to see what others think about that. If it turns out to be an improvement, please address the comments.
beforeEach( | ||
fakeAsync(() => { | ||
// The menu needs a click to render the dropdown! | ||
const dropDownToggle = fixture.debugElement.query(By.css('.dropdown-toggle')); | ||
dropDownToggle.triggerEventHandler('click', null); | ||
tick(); | ||
fixture.detectChanges(); | ||
tableActionDebugElement = fixture.debugElement.query(By.directive(TableActionsComponent)); | ||
toClassName = TestBed.get(TableActionsComponent).toClassName; |
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.
toClassName
is only used once. I don't think it deserves a let toClassName
and I don't see the benefit of reassigning the method to the variable on each iteration, despite the fact that there's only one iteration.
tableActionDebugElement
is also only used once.
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.
I assigned them in the before each because they were initiated first inside the loop of the test. Now they are only initiated once and not 9 times (9 actions).
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.
I think the point is about the scope. toClassName
was only used once in the test and just appeared where it was used, namely the test table actions in submenu
describe block. It was clear where the method belonged to. Other parts of the code couldn't call or access toClassName
and they didn't need it. And they still don't need it. That's why I think it belongs in the block where it was before. I actually think the provided encapsulation of the blocks is one of the purposes of the describe blocks. If that function was used in other blocks, we could add it where you've added it. Or put two describe blocks into another block. Whatever makes sense in that situation.
If you're concerned about TestBed.get(TableActionComponent) being repeatedly called, I'd like to suggest you to use a variable tableActionComponent
instead:
it('has menu entries disabled for entries without create permission', () => {
const tableActionComponent = TestBed.get(TableActionsComponent);
component.tableActions
.filter((tableAction) => tableAction.permission !== 'create')
.map((tableAction) => tableAction.name)
.map(tableActionComponent.toClassName)
.map((className) => getMenuItem(`.${className}`))
.forEach((debugElement) => {
expect(debugElement.classes.disabled).toBe(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.
As this describes only has one test, no other tests can access it. But I could add some more tests to test if the right mark action is disabled or enabled depending on the osd state, which would need all the functions and the beforeEach
as it is, too. This would make this describe not only testing the initial template state with no selection.
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.
@Devp00l just for my understanding:
I assigned them in the before each because they were initiated first inside the loop of the test. Now they are only initiated once and not 9 times (9 actions).
I guess you comment relates to performance?
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.
I'm ok with extracting toClassName
from the map, but since it is a "static" method, I think we can just define let toClassName = TestBed.get(TableActionsComponent).toClassName;
in the describe scope.
No need to assign it in a 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.
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.
Okay, thanks for clarification here. From my point of view is performance < readability/cohesion for test code.
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.
@tspmelo I've tested it it works with the old test, and the test was green, but that was a false positive return, as no expect got run because the function was undefined. Additional I have run only the single it
, if I run the describe
surrounding it
, it will fail with Error: Cannot call Promise.then from within a sync test.
. That means the the only place to keep it is inside test methods as TestBed
will be out of scope otherwise.
But I've found a way to do it without TestBed
👍
.forEach((debugElement) => { | ||
expect(debugElement.classes.disabled).toBe(true); | ||
}); | ||
it('has all menu entries disabled', () => { |
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.
The new code removes a feature of the old one: It doesn't take into account that actions with create
permissions aren't supposed to be disabled by default. Can you please re-add it?
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.
There is no create
action. I was looking for a create action but didn't found one, this means this test description is misguiding, so I changed it.
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.
That's right, it has been removed and will be implemented with a later PR. I know. We can re-add it later, but due to the structure of the dashboard, especially the TableActionComponent and the way the permissions are grouped to create, read, update and write, it is very unlikely that this code has to be touched if any "action" with create permission will be added. IMO it makes perfectly sense to keep it and that it doesn't need to be touched.
But if that doesn't convice you and your desire is a temporary removal of the filter, I'd like to suggest you to simply remove the line that filters the code based on their permissions and change the description of the it
function accordingly.
Remove
.filter((tableAction) => tableAction.permission !== 'create')
adapt the description and it would look like
it('consists of disabled menu entries', () => {
component.tableActions
.map((tableAction) => tableAction.name)
.map(TestBed.get(TableActionsComponent).toClassName)
.map((className) => getMenuItem(`.${className}`))
.forEach((debugElement) => {
expect(debugElement.classes.disabled).toBe(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.
If you add a create action simply change .toBe(true)
to .toBe(action.permission === 'create' ? undefined : 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.
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.
New code seems a bit simpler.
I'm OK with keeping the filter
, but we can also just add it later.
No need to change the expect condition.
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.
My suggestion to change the expected condition is that all items will be tested so that the create
action (which not exists yet) will be tested too. If we would not include it here and it would be disabled which it shouldn't be, we would never now if we filter those actions out.
let tableActionDebugElement; | ||
let toClassName; | ||
|
||
const getActionClasses = (action) => |
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.
Can you please add a proper type to action
?
965378e
to
7d2cd5c
Compare
.forEach((debugElement) => { | ||
expect(debugElement.classes.disabled).toBe(true); | ||
}); | ||
it('has all menu entries disabled', () => { |
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.
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.
Other than #24824 (comment), lgtm.
7d2cd5c
to
d439e82
Compare
Fixes: https://tracker.ceph.com/issues/36616 Signed-off-by: Stephan Müller <smueller@suse.com>
d439e82
to
4971b2d
Compare
@p-na refactored it as discussed 👍 |
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
As discussed #24606 (comment) , the test should be simplified for a RFC PR.
Fixes: https://tracker.ceph.com/issues/36616
Signed-off-by: Stephan Müller smueller@suse.com