Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add select2sortable example #82

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

tdamsma
Copy link
Contributor

@tdamsma tdamsma commented Aug 30, 2020

Together with Pylons/deform#468, follow up of Pylons/deform#465

@stevepiercy
Copy link
Member

@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!

@tdamsma
Copy link
Contributor Author

tdamsma commented Aug 31, 2020

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

@stevepiercy
Copy link
Member

@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.

@tdamsma
Copy link
Contributor Author

tdamsma commented Sep 1, 2020

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.

@stevepiercy
Copy link
Member

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.

@tdamsma
Copy link
Contributor Author

tdamsma commented Sep 1, 2020

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

@stevepiercy
Copy link
Member

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.

@tdamsma
Copy link
Contributor Author

tdamsma commented Sep 1, 2020

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

SeleniumHQ/selenium-google-code-issue-archive#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

@stevepiercy
Copy link
Member

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

@stevepiercy
Copy link
Member

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.

@tdamsma
Copy link
Contributor Author

tdamsma commented Sep 1, 2020

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.

@stevepiercy
Copy link
Member

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
Copy link
Member

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

@stevepiercy
Copy link
Member

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.

Base automatically changed from master to main January 19, 2021 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants