deformdemo icon indicating copy to clipboard operation
deformdemo copied to clipboard

add select2sortable example

Open tdamsma opened this issue 4 years ago • 14 comments

Together with https://github.com/Pylons/deform/pull/468, follow up of https://github.com/Pylons/deform/issues/465

tdamsma avatar Aug 30 '20 19:08 tdamsma

@tdamsma this PR needs a functional test, following this example: https://github.com/Pylons/deformdemo/blob/master/deformdemo/test.py#L2426

Then it looks like both PRs will be good to merge. Thank you!

stevepiercy avatar Aug 30 '20 23:08 stevepiercy

So it was a real pain to write some tests. Two tests are decorated with @unittest.expectedFailure. One because I can't get Selenium to do a drag and drop, can't figure it out so I gave up. The other is a suspected bug in the js select2sortable implementation, which I uncovered during writing of the test. So slightly disappointing, another reconfirmation what a huge time sink frontend web tests can be, for me at least

tdamsma avatar Aug 31 '20 19:08 tdamsma

@tdamsma how do you run the tests? I get /Users/stevepiercy/.pyenv/versions/3.6.8/lib/python3.6/unittest/case.py:550: RuntimeWarning: TestResult has no addExpectedFailure method, reporting as passes RuntimeWarning). I assume you use pytest, but need details.

stevepiercy avatar Sep 01 '20 05:09 stevepiercy

indeed I used pytest, was not aware that nose does not support addExpetectedFailure (as it is in unittest / the standard library). So I added a test that does not pass now (due to selenium issues I don't understand). With pytest this test XFAILS, I naively expected that would work for nose to,

I could of course just remove that test, but I would prefer to leave it in (and allow is to fail). How would you like me to fix this? I could also remove the assert so it always passes, that also feels kind of wrong.

tdamsma avatar Sep 01 '20 07:09 tdamsma

Are you talking about this action? https://github.com/Pylons/deformdemo/pull/82/files#diff-76727a4f38cfc4c03608a2909c2f8c02R2736-R2738

I'm looking into that one. Apparently Selenium does not find the element to be clicked and dragged, and that's the whole point of this plugin, so we should get that test to pass.

stevepiercy avatar Sep 01 '20 07:09 stevepiercy

I'm looking into that one.

Great, that is the issue exactly. And I think I got it working at one point in the development, but then I refactored a bit and could not get it back to working. No Idea what was going on, so I gave up. Also because I had no idea where to look other that the browser itself. I didn't think to look for Selenium debug logs, is that where you are looking? Anyways your help is much appreciated

tdamsma avatar Sep 01 '20 07:09 tdamsma

I looked at the output of running $TOX -e functional3 -- deformdemo.test:Select2SortableWidgetTests.test_adjust_order, as well as a print("elements: ", el1.text, el2.text, el3.text) as a sanity check.

elements:  Chipotle Jalapeno Habanero
F
======================================================================
FAIL: test_adjust_order (deformdemo.test.Select2SortableWidgetTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/stevepiercy/projects/deform/deformdemo_functional_tests/deformdemo/test.py", line 2746, in test_adjust_order
    ['Jalapeno', 'Chipotle', 'Habanero'],
AssertionError: Lists differ: ['Chipotle', 'Jalapeno', 'Habanero'] != ['Jalapeno', 'Chipotle', 'Habanero']

First differing element 0:
'Chipotle'
'Jalapeno'

- ['Chipotle', 'Jalapeno', 'Habanero']
+ ['Jalapeno', 'Chipotle', 'Habanero']

I also run the demo, just to make sure that when I manually do the operation I get what I expect.

stevepiercy avatar Sep 01 '20 07:09 stevepiercy

I am not sure if this may affect this demo, apparently selenium does not support drag and drop in html5:

https://github.com/seleniumhq/selenium-google-code-issue-archive/issues/3604 https://elementalselenium.com/tips/39-drag-and-drop

But honestly I am not even sure how to tell if this demo is html5 or not, I have never been concerned with it before

tdamsma avatar Sep 01 '20 07:09 tdamsma

It is HTML5. I found another more recent issue that says it is a problem with webdriver and provides a workaround.

stevepiercy avatar Sep 01 '20 07:09 stevepiercy

Well, I gave it my best shot with those workarounds, but both failed. I even put a time.sleep(20) in there to give me time to manually do the drag and drop, and then the test actually passed. That's very frustrating.

I also tried with Chrome and chromedriver and got the same result.

I finally tried the example provided in the linked issue, using bormando's script, and that actually worked. Therefore I point the finger at the Select2Sortable plugin itself as the cause of not being testable.

As an alternative, I would suggest trying another implementation. It seems that they all are incomplete solutions, though.

If it cannot pass functional tests, we can put it into the custom widgets bucket instead of Deform core, and note that we cannot verify draggability.

stevepiercy avatar Sep 01 '20 11:09 stevepiercy

Well, I gave it my best shot with those workarounds, but both failed. At least we tried, how disappointing. As we have both done a lot of manual testing (and I even found and solved a bug whilst doing do) we could also conclude it pretty much works, remove/disable the test and call it a day. I recall it was quite a search whenever I made this a few years back through stale github repos and abandoned jQuery plugin sites to find something that I could get working, I don't feel to excited about going down that rabbit hole again to find an alternate implementation.

If you insist I'll move it to the custom widgets bucket though as I see it the custom widgets should also be tested, so not being testable is not really the criterion. It is more does it fit in Deform core or not, and this widget certainly would fit in.

tdamsma avatar Sep 01 '20 19:09 tdamsma

Until the draggable feature, its primary reason for adding the plugin, can be tested automatically, it won't get into core Deform. Reason being that we must be able to verify that other changes to Deform do not cause the widget to fail. Sorry, but that is a hard and fast rule that cannot be compromised.

However I think we can make that happen without too much effort, and it might be better in the long run for future maintenance. I would use https://JSFiddle.net to do a quick demo of the various implementations to determine which make the cut for further testing. That's not too heavy a lift. I'll do one shortly, and add to this issue with screenshots.

Then we could try implementing those that make the cut, testing them with selenium in Deform and deformdemo.

As far as feature requirements, I have identified two so far:

  • Select2 multiple selected items are draggable.
  • Compatible with Bootstrap 4, and optionally Bootstrap 3.
  • Testable via Selenium.

Not a requirement:

  • Select2 draggable <option>s (re-ordering of <select> items).

Are there other requirements that should be considered? I'm willing to pitch in, and not throw in the towel yet.

stevepiercy avatar Sep 02 '20 03:09 stevepiercy

Here's a base jsfiddle: https://jsfiddle.net/stevepiercy/js8t26k5/4/

stevepiercy avatar Sep 03 '20 12:09 stevepiercy

I tried all of them. I give up. Sorry, @tdamsma, but without a functional functional test to cover the draggability, I cannot bring this into Deform core.

We can preserve the work done thusfar, and move it into the custom widgets folder.

stevepiercy avatar Sep 03 '20 13:09 stevepiercy