Implement strategy to test modal that prevent user from leaving unsaved changes
The following discussion from !52458 (merged) should be addressed:
-
@f_caplette started a discussion: (+2 comments) Thought/Issue Preface: This is a big comment, but it's not a big critic.
I think the tests are giving a false sense of confidence at the moment and I'd like to find a better way to do this.
The first one is that we shouldn't test
hasUnsavedChanges
property directly has this is an implementation detail. Changing how we handle that property shouldn't break the test, but it will. Currently we knowhasUnsavedChanges
is bound to the modal, but this could change for a number of reasons. If tomorrow someone added a new propertyshowModal
and passed that toconfirm_unsaved_changes_dialog
component instead ofhasUnsavedChanges
, all the tests would still pass😅 Instead, we should try to test the user facing experience, which is to see the modal if I have unsaved changes, and not see it if I don't. The how we show it doesn't matter.
So we should try to avoid accessing the
vm
to set and test properties as it lead to more brittle tests and are harder to maintain. The question then is how we could test that the modal is shown since this is a browser implementation?I have a couple of suggestions and let me know which one you think make the most sense.
- Leave the tests as is and open a follow-up issue to investigate that. Perhaps with the refactor I am working on it will open new options.
- Try to assert the user facing experience. This is tricky and would perhaps require some digging into how we can check that the notification is shown
- Remove the tests for now and write them later after my refactor. I don't think this would be unreasonable since we are already testing the component itself. this would also include a follow-up issue.
If we go with option 1, there would be a couple of other comments I would make on how we should restructure the tests. I have left them on the lines they concern.
This is a big paragraph but it's really not that big a deal
😄 It's more about internal standards in Pipeline Authoring group and making sure you know that we usually don't assert properties in our group and instead rely on components. If, like in this case, it feels like the only valid testing solution, then perhaps we need to investigate deeper or accept that this is the only way to test this.Because of how much the refactor changes the whole structure, I would skip them personally but let me know!