1967 component tests content view #2169
Conversation
There was a problem hiding this comment.
hey @Mogge, I really love your thoroughness in testing here, but I would love it if you could break these tests into common set up in beforeEach blocks, followed by it blocks with one expectation, only more if unable to test some outcome without multiple.
| .attributes('to'), | ||
| ).toBe('/post-menu-delete') | ||
| items.at(1).trigger('click') | ||
| expect(openModalSpy).toHaveBeenCalledWith('delete') |
There was a problem hiding this comment.
As we talked about briefly on Discord @Mogge, please try to avoid multiple expectations in the same it block... sometimes, it's kinda unavoidable, but in this case, I think it's not.
you can add a beforeEach inside describe contribution, which handles similar set up, then have one expectation per it block, describing what you are testing with the expect
| .attributes('to'), | ||
| ).toBe('/comment-menu-delete') | ||
| items.at(1).trigger('click') | ||
| expect(openModalSpy).toHaveBeenCalledWith('delete') |
There was a problem hiding this comment.
I reduced the expectations to only one per it block. Now I am only checking that the methods are called correctly, and skipped all the tests of text, number of items etc.
| .attributes('to'), | ||
| ).toBe('/report-contribution-title') | ||
| items.at(0).trigger('click') | ||
| expect(openModalSpy).toHaveBeenCalledWith('report') |
There was a problem hiding this comment.
instead of setting up the method openModal as a spy, can we set up $store.commit as a mock, and check that is called with specific data?
There was a problem hiding this comment.
I tried, but it is getting very complicated because y also have to mount the Modal component. I think we have to rely on the tests of the Modal component that the commits are called correctly (which as far as I can see is the case). I ensure in the tests of ContentMenu that the parameters are passed to Modal correctly.
There was a problem hiding this comment.
I can see that, and as we discussed today, you are testing it up to one point, and there are other tests that take it further!!
|
I do not have a good feeling with using .at(number) to grab the menu items I want to test. This can end up in some painful work when new items are added. Can we find some convention to pass a properly defined class to the items, which has the only purpose to identify the item in the tests? Is there a way to add these classes only if we are testing? Does a component somehow know if it is tested or running in production? |
|
hey @Mogge can we pair on this? it would be nice to get it merged in, no? |
|
when can we pair on this @Mogge ... I seem to not be available when you are and vice versa... |
Mogge
force-pushed
the
1967-Component-Tests-ContentView
branch
from
c1d3fb0
to
317b335
Compare
|
As |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Resolving conflicts
| .attributes('to'), | ||
| ).toBe('/report-contribution-title') | ||
| items.at(0).trigger('click') | ||
| expect(openModalSpy).toHaveBeenCalledWith('report') |
There was a problem hiding this comment.
I can see that, and as we discussed today, you are testing it up to one point, and there are other tests that take it further!!
🍰 Pullrequest
Issues
Todo