Cypress test edit action in dashboard actions list NB: OLD - these changes will be integrated into new PRs and this will eventually be deleted #593
Conversation
denektenina
force-pushed
the
cypress_test_actions_list
branch
from
9b4598d
to
27a374f
Compare
denektenina
force-pushed
the
cypress_test_actions_list
branch
3 times, most recently
from
4a20e45
to
3bce321
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
In Dashboard Actions table, actions can be edited and completed in side panel I recently fixed a bug where newly created notes did not appear in the notes list until the side panel were closed and reopened again. The existing tests did not test that, and this issue were partly opened because of this. Therefore, I considered it important to make a test that covered this. This was difficult to test because the notes call was often slow to return, so I landed on setting a wait for 4 seconds before asserting the existence in the notes list. 4 seconds is because cypress automatically fails any calls that take longer than that to return, making it consistent. It makes the tests slower since many of them are waiting longer than they have to, but I think this is important to test. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Can't do a proper review as it's too crowded here already, and all of us have very different opinions about the testing framework (we might need to add contributing guidelines), so just a couple of comments that I believe I didn't try fighting anyone else before.
| editActionDialog.completeActionConfirmButton().should('exist') | ||
| editActionDialog.completeActionCancelButton().should('exist') | ||
| } | ||
| it('Action can be edited', () => { |
There was a problem hiding this comment.
Something important for me: are your newly added tests actually interesting enough to be run every single time?
My original idea was just to add randomization of entry point to the existing complete/edit test as this is not actually any new functionality. Now these tests just repeat!
This might however be another point where we all think very differently. I honestly can't even do a review/look at the tests now, because everything is duplicated and I fiercely, fiercely hate duplication. *angry cat noises*
There was a problem hiding this comment.
After our talk yesterday I have gone through and reassessed my checks, so I think I can defend the checks I am doing now, and have removed others. Please review again and resolve if you are happy :)
There was a problem hiding this comment.
No, I really am not 😄 so I will try to push my point one more time.
As I said, this is quite important for me. You duplicate same test 3 times. I can live with it running without randomization, but the fact that code is copied - makes me sad.
As you see, action tests become a big, big mess with regards to current amount of failures we see on the CI. Code duplication will only make it worse.
That was my second and final attempt to convince you to unify the tests 😄 If you don't see a way to rewrite these tests to make them not duplicated and tidy, I give up. Apparently other reviewers didn't believe it to be important, so I am outnumbered 😄
P.S. Failures in repeated tests might have given me the idea about what's wrong, so I am a little bit more warmed up to this today 😄
There was a problem hiding this comment.
If this is still relevant in the newest PR, please show me where the duplicates are. You say I "same test 3 times", is this in the complete-tests or other places? If in the complete tests, we can fix this for my new PR. If not, we can fix in a later PR.
If you are referring to lines like:
editActionDialog.checkIfCompletedInView(action.completed)
editActionDialog.completeActionButton().click()
editActionDialog.checkIfCompleteConfirmViewVisible(true)
being repeated in the same order several places, please specify if you argue for removing some of the lines because it is not needed to check, or if you argue for placing them in a separate function and reusing this to avoid code duplicate.
What I meant in my comment above is that I did unify the "complete with" and "complete without" comment-tests. This removed a lot of duplicates. I think cancel complete differs, but please argue if you think something should be changed there. If this is too much to explain to me in writing, we could maybe have a call and make the changes together?
There was a problem hiding this comment.
Thanks for trying to explain this! ❤️
New PR at the moment doesn't introduce more tests, so it is not relevant to it. And currently I am happy with the "check" functions. And in general I like new PR very much ❤️
By duplicates I mean
- complete/edit action from workshop/follow up
- complete/edit action from Actions Tab on Follow Up
- complete/edit action from Dashboard
The flow of these actions is repeated. The meaning of these tests is repeated. That's what I don't like.
There were 3 options for me how to deal with that
- Randomization (you randomize the way you enter the dialog) - you run it just once from random entry point
- For loop, like for roles - you run it 3 times, for each way to enter the dialog (as with randomization, the whole flow is still covered by 1 code sequence and 3 entry functions). I do not like this approach, as it will take long to complete and doesn't introduce much value to the tests.
- wait until we establish this "lightweight second level" of Petters test strategy where we will only check from all 3 places that 1) edit link is present 2) Complete action button is present 3) whatever else we need to check (that notes number is as expected?) without actually doing or checking anything else
I am at the moment for a mix of approaches 1 and 3, but that requires Petters vision implemented as example for some tests and our discussion on randomization to happen
denektenina
force-pushed
the
cypress_test_actions_list
branch
2 times, most recently
from
3d043a7
to
ba204b5
Compare
denektenina
force-pushed
the
cypress_test_actions_list
branch
from
ba204b5
to
b01d707
Compare
| editActionDialog.completeActionConfirmButton().should('exist') | ||
| editActionDialog.completeActionCancelButton().should('exist') | ||
| } | ||
| it('Action can be edited', () => { |
There was a problem hiding this comment.
No, I really am not 😄 so I will try to push my point one more time.
As I said, this is quite important for me. You duplicate same test 3 times. I can live with it running without randomization, but the fact that code is copied - makes me sad.
As you see, action tests become a big, big mess with regards to current amount of failures we see on the CI. Code duplication will only make it worse.
That was my second and final attempt to convince you to unify the tests 😄 If you don't see a way to rewrite these tests to make them not duplicated and tidy, I give up. Apparently other reviewers didn't believe it to be important, so I am outnumbered 😄
P.S. Failures in repeated tests might have given me the idea about what's wrong, so I am a little bit more warmed up to this today 😄
| seed.plant() | ||
| }) | ||
|
|
||
| it('Actions are visible on Dashboard Actions Tab', () => { |
There was a problem hiding this comment.
I am so not sure about this test in general... The main goal of the dashboard is to "only display all the actions assigned to the current user across the projects", but what we essentially check is "some actions assigned to current user in the project we are in are displayed"
- if we really want to test functionality of the dashboad, we need to introduce actions from a different project
- we probably need to make sure that all the actions in the table are assigned to the user and there are no unexpected actions are present (we in theory could have checked that in other view tests as well, but I believe danger there is lesser, though still up for discussion)
- it would be great to also make sure that all the actions assigned to the user are present here (again, can apply to other view tests as well, but that would be difficult to implement)
- (probably fine) dashboard tab displays all the actions assigned to the user. It might happen that there are too many actions present, so the actions we are searching for might not be visible on the page. Probably (?) cypress should deal with this.
So I think that this is also something to be discussed as a general concept if we want/need it in our tests as checking that expected and actual map 1 x 1 would need additional graphql query calls. (Separate project/user would work only if we agree to test this on special occasions only)
Basically awesome point for discussion, but if you are tired of them and no discussion happens, test in current form can stay, but I would prefer it marked with comments "here are possible improvements"
| if (putAllActionsOnEditor) { | ||
| preassignedActionValues.assignedTo = editor | ||
| } |
There was a problem hiding this comment.
To the dashboard dicsussion:
If we could have a graphql call "return me all actions assigned to the user" or, in a perfect world have separate projects with separate user where we are in full control which actions are created (I actually think we can do it!!), we wouldn't need this hack.
| @@ -83,8 +84,7 @@ describe('Actions', () => { | |||
| const actionProgression = faker.random.arrayElement([Progression.Workshop, Progression.FollowUp]) | |||
|
|
|||
| beforeEach(() => { | |||
| ;({ seed, existingAction, existingNotes } = createEvaluationWithActionsWithNotes({ title: 'Just some random action' })) | |||
| editor = faker.random.arrayElement(seed.participants) | |||
| ;({ seed, editor, existingAction, existingNotes } = createEvaluationWithActionsWithNotes({ title: 'Just some random action' })) | |||
There was a problem hiding this comment.
replace editor with user and login in the same way as other tests in actions_spec.
only return seed in before each.
Move getting of existingAction & existingNotes into test itselg
There was a problem hiding this comment.
Basing my new PR on your newest PR so I think this is fixed
denektenina
changed the title
|
Closing this because the tests have been covered in other PRs. The only thing still missing is potentially testing that notes appear in the list right after adding (without reload) in all three views. It has been debated a lot whether or not we should test all kinds of editing of actions and notes with reload in all three side panels, but we have landed on doing it, and it will be done in other PRs. |
Made cypress tests for actions in actions table on dashboard and follow up page. Testing the following:
Notes / improvements