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

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

Closed
wants to merge 25 commits into from
Closed

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

wants to merge 25 commits into from

Conversation

sydoluciani
Copy link
Contributor

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

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sphinx 3.0.x's advice for how to resolve the duplication is misleading here. There are actually two instances of get_root that get pulled into the rendered docs, one of which does not have a link to it.

There are at least three ways to resolve the error, each of which has its own impacts.

  1. Remove the duplicate entries .. automethod:: translate and .. automethod:: get_root on lines 20 and 22. I prefer this option because it preserves the index entries and links to the methods, removes the duplicates from the rendered docs, and sorts all methods alphabetically.
  2. Add the methods to their class's :exclude-members: get_root, translate options. This option has the same affect as option 1, except it preserves relative importance of the listing of methods. It's the best option if you don't care about alphabetic sorting, and prefer some logical or "importance sorting".
  3. :noindex: merely removes the link to and index entry of the duplicate item, leaving it duplicated in the rendered docs. This is definitely not the correct option.

Also assuming you are hitting the other warnings:

deform/field.py:docstring of deform.Field:87: WARNING: term resource registry not found in case sensitive match.made a reference to Resource registry instead.

You can solve this with -D suppress_warnings=ref.term in tox.ini. See discussion at Pylons/pyramid#3577 (comment). IOW, I want to see those specific warnings when I do make html, but not when building in tox or CI. This type of warning is only important if you care about things like MySQL the database versus mysql the command and each term and its definition should be unique in the Glossary. We don't have anything like that in any of our docs, so it is OK to suppress the warning when building the docs, but it is helpful for the developer in case there is some typographical conflict.

@stevepiercy
Copy link
Member

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
Copy link
Contributor Author

sydoluciani commented Apr 28, 2020

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

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
Copy link
Contributor Author

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

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

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

stevepiercy commented Apr 28, 2020

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

@sydoluciani
Copy link
Contributor Author

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

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

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: #397 (review)

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

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
Copy link
Contributor Author

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

stevepiercy commented Apr 28, 2020

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

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
Copy link
Contributor Author

sydoluciani commented Apr 28, 2020

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

@stevepiercy
Copy link
Member

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
Copy link
Contributor Author

sydoluciani commented May 5, 2020

@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
Copy link
Contributor Author

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

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

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

According to #305 it seems like it could be bumped.

@stevepiercy
Copy link
Member

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

stevepiercy commented May 6, 2020

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.

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
Copy link
Contributor Author

sydoluciani commented May 6, 2020

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

sydoluciani commented May 6, 2020

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

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

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

@sydoluciani
Copy link
Contributor Author

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

@stevepiercy
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

sydoluciani commented May 7, 2020

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

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

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

@sydoluciani
Copy link
Contributor Author

sydoluciani commented May 8, 2020

@stevepiercy thats great.

I get the same result either using Xvfb or Xwindows: FAILED (errors=5, failures=9) , no skiped though.

I am working on errors and stuck with first one since yesterday, you can start with failures.
in both cases and in local development, it is best to switch from Xvfb to Xwindows and view the test as it progress. travis setup stay the same, running Xvfb.

With a quick pick at failures, it might be just as simple as number arrow keys being sent too many, too few or maybe not being sent at all, and that can be easily verified by viewing at browser.

@sydoluciani
Copy link
Contributor Author

sydoluciani commented May 8, 2020

I just did a quick test, and number of left arraw keys being sent is correct, but some how the sent number being lost in first position, it works in other positions.

FAIL: test_type_bad_input (deformdemo.test.TextInputMaskTests)
Compare:
findid("deformField1").send_keys(11 * Keys.ARROW_LEFT)
findid("deformField1").send_keys("00")
With:
findid("deformField1").send_keys(10 * Keys.ARROW_LEFT)
findid("deformField1").send_keys("00")

And FAIL: test_submit_filled (deformdemo.test.MoneyInputWidgetTests)
caused by wrong sequence of key strokes, below sequence works in real browser but not in selenium:
findid("deformField1").send_keys("1")
findid("deformField1").send_keys(5 * Keys.ARROW_LEFT)

@sydoluciani
Copy link
Contributor Author

The good news is, this pull requests is using Bootstrap 4, and getting the same number of errors your pull request generated, so once we fix functional tests then the deform upgrade is done.

The bad news is, from looking at the errors or failures, problem mainly is Selenium and how it is dealing with browser. everything works when working with actual browser.

@stevepiercy would you like to complete test against deform/schema.py and push your pull request, by then I might find solution to some of the functional tests as it seems most of them follow the same pattern, and we have to find a work around.

Thanks

@sydoluciani
Copy link
Contributor Author

sydoluciani commented May 9, 2020

I have fixed 7 of the 9 functional test failures.
Selenium tests need to use "selenium.webdriver.common.action_chains" to access some of the fields, otherwise Selenium won't interact properly with the field.

Had to send a new pull request to Deformdemo to reflect changes in Deform functional tests.

https://github.com/sydoluciani/deformdemo/blob/bootstrap4_test/deformdemo/test.py#L84
https://github.com/sydoluciani/deformdemo/blob/bootstrap4_test/deformdemo/test.py#L90

@stevepiercy Deformdemo tests are using Firefox 45, and they are failing to start xvfb.
Deform is starting xvfb using systemctl and deformdemo starting it from /etc/init.d.

@stevepiercy
Copy link
Member

@sydoluciani I think we need to bring deformdemo up to the same environment as deform, per the change in how xvfb is started in Travis since Jan 2019. We also need to drop Python 2 from deformdemo to align with deform.

@sydoluciani
Copy link
Contributor Author

Will be completed in #403.

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.

None yet

4 participants