Pylons / deform

A Python HTML form library.
Other
416 stars 160 forks source link

Replaced bootstrap.min.css and bootstrap.min.js files with version 4. #397

Closed sydoluciani closed 4 years ago

sydoluciani commented 4 years ago

355

First step in upgrading bootstrap 3 to bootstrp 4. Replaced bootstrap.min.css and bootstrap.min.js files with version 4.

stevepiercy commented 4 years ago

As far as those two failing tests, I'm not entirely sure how to fix them. Sometimes a sleep will give the widget time to render in the DOM so that the xpath can actually find it. Selenium might have some screenshotting thing of the failing tests to see what it sees when it runs the tests. Anyone got any idea of how to fix these two tests?

sydoluciani commented 4 years ago

@stevepiercy I have removed both get_root and translate from api.rst. There is another error related to coverage which I would like to address before working on failed fucntional tests.

https://travis-ci.org/github/Pylons/deform/jobs/680376151#L327

No source for code: '/home/travis/build/Pylons/deform/deform/checkbox_32bb822ccaed554768253405d5135c00.py'.

suggested fix is adding -i, but to which line ?

stevepiercy commented 4 years ago

I just saw a similar error in pyramid_zcml in the past week, but that suggested fix is not quite right. The best fix is to put into .coveragerc an omit line. Example: https://github.com/Pylons/pyramid_zcml/blob/master/.coveragerc#L4

In this case, omit = deform/checkbox_32bb822ccaed554768253405d5135c00.py ought to do it.

sydoluciani commented 4 years ago

After some research, apparently coverage error 'No source for code' is related to a deleted file and related left oever .pyc, either in local repository, or in travis docker test environment server. there should be a pre-start-up script to clean all *.pyc files before starting the test environment, otherwise this error randomly shows up in coverage test.

stevepiercy commented 4 years ago

Got link to research?

I don't think it is a .pyc in the repo by virtue of *.pyc in .gitignore. I don't know about Travis. The hash in the filename might be a clue. Did you try my suggestion in .coveragerc?

stevepiercy commented 4 years ago

There is a duplicate entry in glossary.rst for renderer, both by mcdonc, causing the docs build to fail. Suggest merging the second into the first with this:

   renderer
     A function which accepts a logical template name and a set of
     keywords, with the signature ``(template_name, **kw)``, and
     which returns the rendering of a deform widget template.
stevepiercy commented 4 years ago

Try copying the omit under the [run] block, as in the pyramid_zcml example.

sydoluciani commented 4 years ago

Replaced the renderer as you suggested, but there is bigger problem in rebuilding the doc. https://travis-ci.org/github/Pylons/deform/jobs/680443839#L386

term resource registry not found in case sensitive match. made a reference to Resource registry instead.

How about ignoring the warnings instead of considering it error ?

And there are random missing files started to show up under the coverage. What about removing --show-missing in: coverage report --show-missing --fail-under=100

stevepiercy commented 4 years ago

Actually we might now need to specify the source for running coverage on deform. Example from pyramid_zcml:

[run]
source =
    pyramid_zcml
omit = pyramid_zcml/pyramid_zcml_tests_viewdecoratorapp_views_templates_foo_mak

[paths]
source =
    pyramid_zcml
    */site-packages/pyramid_zcml

By the way, are you running tox -e coverage locally? It works great with pyenv to test on multiple Pythons.

stevepiercy commented 4 years ago

term resource registry not found in case sensitive match. made a reference to Resource registry instead.

How about ignoring the warnings instead of considering it error ?

See earlier comment: https://github.com/Pylons/deform/pull/397#pullrequestreview-401514094

And there are random missing files started to show up under the coverage. What about removing --show-missing in: coverage report --show-missing --fail-under=100

That will have no effect on the actual failures, as it is merely an output format. See coverage docs and coverage summary.

stevepiercy commented 4 years ago

What is going on with that coverage failure? 😕 Note that the file name changes between runs.

I think my earlier suggestion, try to fix with omit, is not correct. Perhaps try specifying the source instead?

sydoluciani commented 4 years ago

Omit is correct, but more files are showing up and since they are in deform directory can not use wild card on directory. not sure if test procedure is creating them or they are simply left over .pyc from last build.

stevepiercy commented 4 years ago

That's why I suggest specifying source per the example above. I don't think codecov is the correct setting. That string appears nowhere in the repo. I haven't tried it yet, but that's my guess.

stevepiercy commented 4 years ago

Actually, why are we even bothering to cover Python 2.7? Let's drop it. Then we don't have to merge py27 with py36 coverage for overall coverage, and can just look at py36. Problem solved! Compare the branches master and 1.10-branch in Pyramid for examples.

sydoluciani commented 4 years ago

Removed python 2.7 and basepython and passed the test. Only left the selenium test errors.

stevepiercy commented 4 years ago

Why did you modify the translation files as part of this PR?

stevepiercy commented 4 years ago

Coverage is now misconfigured. See previous suggestions to correct it.

sydoluciani commented 4 years ago

I think after changing the basepython in tox.ini all those translation files changed.

Had to remove Python 3.6 basepython since I don't have Python 3.6 and changing the basepython to 3.7 failed on all tests.

You are right, there is missconfiguration, adding --fail-under=100 throws error again.

stevepiercy commented 4 years ago

Please revert those changes to the base Python and translation files in tox. Developers must adapt their systems to the project, not the other way around, so we continue to support various Python versions.

Coverage is misconfigured because it returns a false positive: although Travis says it passes, it actually does not because you removed critical configuration. See this line: https://travis-ci.org/github/Pylons/deform/jobs/680777570#L392 and you will see coverage is at 58% which should cause the build for coverage to fail.

If you need help with setting up your system to allow multiple Pythons, we are glad to help out. Many of us use pyenv to manage multiple Pythons. You can hit us up in IRC or Keybase.

sydoluciani commented 4 years ago

You are right, after adding --show-missing --fail-under=100, getting the same error. No source for code: '/deform/checkbox_fdc46098de12c7d295ff61cb419e5a84.py'

Either I will revert the changes on translation files, or I will create a new pull request after all tests have been passed.

Thank you for all your help.

I am running virtual environment wrapper and already having Python 3.5, 3.7 and 3.8 in system base, but some how Debian does not have Python 3.6 in its repository any more, or at least there was none when I checked couple of months ago. going to check if I can install Python 3.6 and Python 3.4.

stevepiercy commented 4 years ago

You can skip Python 3.4 because that has been EOLed as well. Thank you for persevering!

stevepiercy commented 4 years ago

@sydoluciani, based on another PR #398 just submitted to deform and its build also failing, your changes should not be triggering the build errors. I think what we ought to focus on first is resolving the build errors, so you can focus on only migrating to BS4. I'll open another PR to work through some of them, taking some of the work we've done here.

sydoluciani commented 4 years ago

What is going on with that coverage failure? 😕 Note that the file name changes between runs.

I think my earlier suggestion, try to fix with omit, is not correct. Perhaps try specifying the source instead?

@stevepiercy this issue is already reported in coverage issue tracker. please read end of this thread: https://bitbucket.org/ned/coveragepy/issues/98/no-source-for-code https://github.com/nedbat/coveragepy/issues/365

These temp files are being created as part of template rendering process.

deform/checkbox_*.py
deform/dateinput_*.py
deform/form_*.py
deform/mapping_*.py
deform/password_*.py
deform/radio_choice_*.py
deform/**add_many_more_files_to_end_of_this_list**

For example deform/checkbox_fdc46098de12c7d295ff61cb419e5a84.py being created because a template is defined in deform/widget.py: template = "checkbox"

Deform is passing these templates to pyramid_chameleon and further passed to Chameleon to render, and Somehow Chameleon is creating those temporary files in deform module instead of /tmp directory, and this is the problem since Deform module is the source for both run and report directives in .coveragerc.

At the moment omitting files that has digit after underscore seems the only way to bypass the coverage failure on those temporary files.

Thanks

stevepiercy commented 4 years ago

@sydoluciani nice find! @mmerickel and @bertjwregeer are core contributors to Pylons Projects. Perhaps they can also advise on how to definitively resolve this coverage issue?

Most of those solutions involved still supporting Python 2. I'll work on both its and py34's removal today, now that I have wrapped up sprinting on a huge project.

stevepiercy commented 4 years ago

Well, I spent a couple hours trying to run coverage. The coverage command itself runs, but coverage is below 100%, which means it ultimately fails. I've pushed up what I tried in a new PR #400, taking some bits from this PR.

sydoluciani commented 4 years ago

@stevepiercy Thank you.

Tested my suggested regex expression in https://regex101.com/, after copy and paste the file names in text area of that site, and noticed it matches files that should not be omited, this one seems a better version for now until this bug fixes:

deform/*[0-9]*

So if the coverage is less than 100 percent because of these 3 files: deform/schema.py deform/renderer.py deform/compat.py

Then we have to write test for functions that current tests not covering, right ?

sydoluciani commented 4 years ago

Looking at current master .coveragerc comment: # These are being stressed out in deformdemo package omit = deform/schema.py deform/renderer.py

I have not started working on functional test yet, but assuming from previous comment, schema and renderer is being tested by Selenium and under functional test.

missing statements in deform/compat.py deform/compat.py 22 7 68% 26-31, 58

Are lines 26 to 31, which is related to Python 2 and can be ignored for now, and line 58, which is url_unquote, already included in deform/tests/test_widget.py but not sure why it is not found by coverage, there might be some rules in matching tests with statements that is not being followed.

Would you like to accept your branch as is for now, and merge, so I can close this PR, do a new fork from your merged branch, and start working on functional tests ? that would be a bigger issue.

sydoluciani commented 4 years ago

In latest .coveragerc file, there are semicolon begining of some lines, are those # or they should be \; ?

https://github.com/Pylons/deform/blob/drop-py27-py34/.coveragerc

stevepiercy commented 4 years ago

Tested my suggested regex expression in https://regex101.com/, after copy and paste the file names in text area of that site, and noticed it matches files that should not be omited, this one seems a better version for now until this bug fixes:

deform/[0-9]

That doesn't work for me. Compare my results, and not the regex flavor.

So if the coverage is less than 100 percent because of these 3 files: deform/schema.py deform/renderer.py deform/compat.py

Then we have to write test for functions that current tests not covering, right ?

You got it! Once the missed lines have coverage, then the coverage build should pass.

sydoluciani commented 4 years ago

Tested my suggested regex expression in https://regex101.com/, after copy and paste the file names in text area of that site, and noticed it matches files that should not be omited, this one seems a better version for now until this bug fixes: deform/[0-9]

That doesn't work for me. Compare my results, and not the regex flavor.

You are right, I missed deform in my latest regex pattern.

So if the coverage is less than 100 percent because of these 3 files: deform/schema.py deform/renderer.py deform/compat.py Then we have to write test for functions that current tests not covering, right ?

You got it! Once the missed lines have coverage, then the coverage build should pass.

Thank you.

stevepiercy commented 4 years ago

Looking at current master .coveragerc comment:

These are being stressed out in deformdemo package

omit = deform/schema.py deform/renderer.py

I have not started working on functional test yet, but assuming from previous comment, schema and renderer is being tested by Selenium and under functional test.

The Selenium functional tests are a different kind of test, testing the rendered widgets in web browser. coverage covers unit tests. I don't know why they were omitted from coverage. I checked git blame and perhaps @miohtama can elaborate?

missing statements in deform/compat.py deform/compat.py 22 7 68% 26-31, 58

Are lines 26 to 31, which is related to Python 2 and can be ignored for now, and line 58, which is url_unquote, already included in deform/tests/test_widget.py but not sure why it is not found by coverage, there might be some rules in matching tests with statements that is not being followed.

coverage has a feature to ignore specific lines of code, which is exactly what we want to do for running coverage on Python 3. The code will continue to run on Python 2 and not break for current users. At some point, we can gut Python 2 compatible code, but that's another PR.

Would you like to accept your branch as is for now, and merge, so I can close this PR, do a new fork from your merged branch, and start working on functional tests ? that would be a bigger issue.

You can work on my branch now. 😄 Just git fetch upstream; git checkout drop-py27-py34 and you're ready to rock and roll. 🥁 🎸 🎹

In latest .coveragerc file, there are semicolon begining of some lines, are those # or they should be ; ?

Either is acceptable syntax for comments in .ini files. My editor (PyCharm) happens to use ;. 🤷

stevepiercy commented 4 years ago

deform/compat.py now has 100% coverage. Basically Python 2 code was getting missed by coverage, and we were telling coverage to ignore Python 3 code. Toggling coverage to ignore Python 2 code bumped up coverage to 100%.

A test needs to be written in deform/tests/test_schema.py for the class CSRFSchema in deform/schema.py so that it uses the deferred. That should be a nice easy one to fix.

For deform/renderer.py, it allows the developer to override default templates with a custom template. I have no clue how to test this, though, other than through a functional test. I searched git blame, DuckDuckGo, docs, and there's not much documentation for it.

sydoluciani commented 4 years ago

@stevepiercy it's time to upgrade Firefox version 43 to newest version: https://travis-ci.org/github/Pylons/deform/jobs/681569161#L162

Developers won't be able to duplicate this environment on their local server, since Firefox version 43 can't be started on newer versions of Debian. wondering what OS version are you using to start Firefox 43 ?

Tests are throwing errors unrelated to upgrading bootstrap, mostly timing issues, like option is not clickable, There is wait for findid, but not for options within select, and there is problem running tests within tox due to version mismatch between Firefox, geckodriver and Tox.

sydoluciani commented 4 years ago

deform/renderer.py

deform/compat.py now has 100% coverage. Basically Python 2 code was getting missed by coverage, and we were telling coverage to ignore Python 3 code. Toggling coverage to ignore Python 2 code bumped up coverage to 100%.

A test needs to be written in deform/tests/test_schema.py for the class CSRFSchema in deform/schema.py so that it uses the deferred. That should be a nice easy one to fix.

For deform/renderer.py, it allows the developer to override default templates with a custom template. I have no clue how to test this, though, other than through a functional test. I searched git blame, DuckDuckGo, docs, and there's not much documentation for it.

Using Python Mock library, to replace the default template with Jinja2, and test if variable or expression syntax works using Jinja2, or even if the new template directory shows up in template search path ?

stevepiercy commented 4 years ago

@sydoluciani for Firefox v43, I know there is some history behind it but I don't recall what it was. I agree that it should be upgraded, if possible. Perhaps @miohtama @ericof @domenkozar has knowledge of this? I asked in the web sauna Gitter channel, too.

stevepiercy commented 4 years ago

For overriding the default templates, we can use Chameleon. It's not as easy as Jinja2 syntax, but it is not awful and it would be consistent. Otherwise, I think that would be a good way to test the override.

domenkozar commented 4 years ago

According to https://github.com/Pylons/deform/pull/305 it seems like it could be bumped.

stevepiercy commented 4 years ago

Thanks @domenkozar. That nudged me forward for Travis.

It also jogged my memory to look in https://github.com/Pylons/deform/blame/master/CONTRIBUTING.rst#L50 (who wrote that?) 😝 . @sydoluciani try downloading the latest Firefox Extended Support Release (ESR), and following the rest of the instructions in CONTRIBUTING.rst. If it works for both you and me, then we can update that doc.

stevepiercy commented 4 years ago

I tried downloading the FF ESR latest, setting export FIREFOX_PATH=/Applications/Firefox-ESR.app/Contents/MacOS/firefox, and running functional tests, but got a similar error to that mentioned in https://github.com/Pylons/deform/pull/400#issuecomment-624551740

This answer on SO looks promising. It looks like several requirements need to be updated to get it to work.

I've used up my open source time for the day. I'll try to pick this up later, but if anyone definitively fixes it, I'd appreciate it.

selenium.common.exceptions.WebDriverException: Message: Can't load the profile. Profile Dir: /var/folders/fx/p4fx43ts6f1dqgj29c0y20yc0000gn/T/tmpp6pcc3lf If you specified a log_file in the FirefoxBinary constructor, check it for details.
sydoluciani commented 4 years ago

@stevepiercy Selenium version installed in tox environment was different to what it was installed in other virtual environment.

.tox/functional3/bin/pip list | grep -i selenium

Changing selenium<3 to selenium>3 in two files and rerun the test is a success: setup.py
deformdemo_functional_tests/setup.py

If Xvfb is being used, tests still might fail due to exceeding speed on running all tests against pserve server. pserve server become unresponsive due to number of requests it receives, but
That depends on speed and number of CPUs which probably not the case with docker image that is currently being used.

I didn't have such problem using Xwindows, since tests are running slower in Xwindows environment.

sydoluciani commented 4 years ago

I tried downloading the FF ESR latest, setting export FIREFOX_PATH=/Applications/Firefox-ESR.app/Contents/MacOS/firefox, and running functional tests, but got a similar error to that mentioned in #400 (comment)

This answer on SO looks promising. It looks like several requirements need to be updated to get it to work.

Yes, That is a quick guide to install all required software to start Firefox with Selenium webdriver, however FIREFOX_PATH needs to be pointed to firefox executable as well.

And geckodriver can be extracted to different directory instead of moving it to /usr/local/bin/, then add the directory to the PATH, so it cant be found and setting WEBDRIVER to geckodriver to executable file.

I've used up my open source time for the day. I'll try to pick this up later, but if anyone definitively fixes it, I'd appreciate it.

selenium.common.exceptions.WebDriverException: Message: Can't load the profile. Profile Dir: /var/folders/fx/p4fx43ts6f1dqgj29c0y20yc0000gn/T/tmpp6pcc3lf If you specified a log_file in the FirefoxBinary constructor, check it for details.
sydoluciani commented 4 years ago

Thanks @domenkozar. That nudged me forward for Travis.

It also jogged my memory to look in https://github.com/Pylons/deform/blame/master/CONTRIBUTING.rst#L50 (who wrote that?) 😝 . @sydoluciani try downloading the latest Firefox Extended Support Release (ESR), and following the rest of the instructions in CONTRIBUTING.rst. If it works for both you and me, then we can update that doc.

@stevepiercy I have updated the CONTRIBUTING.rst with debian version, can you please go over the installation and see if there is any step missed in procedure.

https://github.com/sydoluciani/deform/blob/master/CONTRIBUTING.rst#preparing-selenium-and-firefox-on-linuxdebian

digitalresistor commented 4 years ago

Hey, instead of trying to make a local browser work, why not update the tests to instead use the Selenium Remote protocol and use a docker container that contains the browser, pre-packed with Selenium server?

For example:

https://hub.docker.com/r/selenium/standalone-firefox

I used this successfully in the past to build a package for use with nightwatch: https://github.com/Crunch-io/nightwatchrun/blob/master/nightwatchrun.sh

What it did is start up a Selenium docker image, then had the test suite connect to said Selenium and do the thing.

The same principle applies here.

Spin up the selenium docker container (which will work nicely in CI too), connect to it, and run tests.

digitalresistor commented 4 years ago

Wrap it all nicely into a docker-compose for the functional tests, even better.

sydoluciani commented 4 years ago

And other Pylon projects can use the same Selenium image for their testing. very good idea.

stevepiercy commented 4 years ago

@sydoluciani is the Docker container something you can work on? I have no idea how to implement @bertjwregeer's suggestion. I'll hold off on my review of the CONTRIBUTING.rst docs revision for now.

sydoluciani commented 4 years ago

We have to fix the failed tests regardless of current setup or using Selenium server.

Currently there is 9 failures and 5 errors which is consistent with previous tests, and I can use your help to fix them.

Let's continue with current setup and upgrade the firefox which is the easier task, then fix the failed tests to merge your branch with master and use bootstrap 4 wihch is the priority.

sydoluciani commented 4 years ago

@sydoluciani is the Docker container something you can work on? I have no idea how to implement @bertjwregeer's suggestion. I'll hold off on my review of the CONTRIBUTING.rst docs revision for now.

Selenium docker can be started on demand within Travis CI before tests start. here is a short instruction: http://witkowskibartosz.com/blog/how_to_integrate_travis_with_selenium_tests_using_docker.html#.XrRHE15Kg_w

Then functional tests has to be modified to instantiate webdriver.Remote instead of webdriver.Firefox to send the tests to remote Selenium-Firefox that just started instead of testing on local Selenium-Firefox.

https://gist.github.com/dzitkowskik/0fc641cf59af0dc3de62#remote-selenium-server Instead of: https://github.com/Pylons/deformdemo/blob/master/deformdemo/test.py#L235

Now the advantage of such setup is having a ready to use Selenium driver and browser, but to take full advantage, one has to setup a Selenium server as a hub, and different selenium nodes to run different browser on different OS to be able to run tests against them, which at that point one would want to have dedicated docker rather than on demand, to run all Pylons projects web tests against the hub instead of individual Selenium setup.

Check out these links: https://www.egrovesys.com/blog/implementing-selenium-grid-in-python/ https://examples.javacodegeeks.com/enterprise-java/selenium/selenium-standalone-server-example/

These are on top of fixing current failed tests that needs to be fixed regardless. Let's continue with local and upgrade the Bootstrap for now.

digitalresistor commented 4 years ago

No, we don't need to set up selenium server or anything along those lines. We can spin up docker containers inside CI, and run tests. Whether that remote is Firefox or Chrome won't matter, that's the beauty of it. I have no interest in managing a selenium anything. Just avoiding the mess we have right now.

stevepiercy commented 4 years ago

@sydoluciani I followed your guidance in your version of CONTRIBUTING.rst but for macOS. I got it to work for me. Good work! And it is reasonably fast. I don't have Linux Debian except in VMs, so I cannot easily verify it, but...

...note that we now put "how to contribute" stuff into contributing.md for Pylons Project projects. I will update that in a separate PR for both macOS and Linux, collecting both of our notes into one file for review.

Now when I run tox -e functional3 I get FAILED (SKIP=12, errors=5, failures=9). How would you like to collaborate on fixing the tests so we don't duplicate efforts?