PROCEED-Labs / proceed

Business Process Management System
MIT License
5 stars 9 forks source link

Process List: Sorting, Columns, One-Click Link #259

Open iaktern opened 6 months ago

iaktern commented 6 months ago
FelipeTrost commented 5 months ago

@iaktern I think adding a drag and drop symbol would make the process list too cluttered, what do you think?

MaxiLein commented 5 months ago

@iaktern I think the selection of Elements in the list via the checkbox should not be tested, as it is part of the antdesign table.

iaktern commented 5 months ago

@iaktern I think the selection of Elements in the list via the checkbox should not be tested, as it is part of the antdesign table.

yes, you're right: we should only test logic coming from our code. But in your case, isn't the selection logic coming from us? e.g., if someone clicks an icon within the row.

MaxiLein commented 5 months ago

@iaktern I think the selection of Elements in the list via the checkbox should not be tested, as it is part of the antdesign table.

yes, you're right: we should only test logic coming from our code. But in your case, isn't the selection logic coming from us? e.g., if someone clicks an icon within the row.

While the selection logic can be set from outside, I still think, it should not be tested as part of the selection of elements within the list.

The selection of an element in the list is given through AntDesign, we just pass it some callback for further functionality - which, currently, is just saving the selection in a state, as well as saving the last clicked element..

I think further added functionality should definitely be tested, however, this should come from the user's perspective, i.e. not checking state changes but rather gui changes associated with it. These test should then test the further functionality that follows / results from the selection. However, they should not test the selection itself, as it is given.

Functionality inside some nested elements such as buttons / icons can and should be tested - I agree - but they are not part of the selection either, but rather doing something else, such as copying or exporting.

As of for the way it is implemented currently, the only way to select an element, is via the 'Select all' shortcut, which has some tests on this branch (WIP), via the checkbox click (handled by AntDesign), or if starting some custom logic that then sets the selection from outside (e.g. exporting a process - which has, and should have its own tests). The latter - I think - should then be tested in tests for the respective functionality rather than a way of selection; otherwise the tests are overlapping.

OhKai commented 5 months ago

@MaxiLein We don't need to test implementation details of AntDesign, but we need to test that we use them correctly (e..g the correct props).

For example, we don't need to test the DatePicker component, that it displays the days of a month in the correct order - that is up to AntDesign. But we need to test that it is configured correctly (for example a max range of dates, show only months, also show day time, etc.). If, for example, we accidentally delete a prop or the name or type of a prop changes in AntDesign and we don't update it, it would be an error on our side that the E2E tests should catch.

For this selection case, AntDesign handles the drawing of the checkboxes, but only if we set the props correctly and store the selection. If we ever have a bug, that overrides the selection state, that needs to be caught by E2E tests. We don't need to test the checkboxes themselves, but we need to test that the selection as a whole works.

But as you said, since we should test user stories in Playwright, it should be enough to test the derived functionality of the modals etc. working with the selected processes, so that should be enough. Because that inherently also covers the selection of processes - it's just important that they are included in the tests somehow.