-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
version bump LEL 2.1.9 #7299
version bump LEL 2.1.9 #7299
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7299 +/- ##
==========================================
+ Coverage 78.11% 80.06% +1.94%
==========================================
Files 97 97
Lines 5598 5622 +24
==========================================
+ Hits 4373 4501 +128
+ Misses 1225 1121 -104
|
We are using Chrome driver version 74, right? chromeOptions = %w(--headless --disable-gpu --no-sandbox --remote-debugging-port=9222 |
Those are the same failed tests I was dealing with before. It's a time problem, they pass local on local but were failing on the server because they took too long. I got them to pass by increasing the wait time. If you increase the number it will probably pass, try 120. plots2/test/system/map_test.rb Line 6 in 4a2da4c
|
OK, thanks so much! Tried that, let's see! |
Hi @nstjean would you have a moment to check this out? Thank you! It didn't seem to help, but is it the same issue? |
That does look like the same issue. I wonder if the new version of LEL is slowing down the map creation? Looking at the code, maybe in the test we can override using the |
Hm, i'll give it a try - would you mind pointing me to the specific line of code? Thank you!! |
Unfortunately it looks like there's no simple way to set that variable in tests, as it currently isn't used. The code is here, but I don't think we want to set the page default to no layers permanently! plots2/app/views/map/map.html.erb Lines 88 to 93 in fbabe28
I played with it on my computer and setting the default to no layers didn't speed up the tests at all, the whole |
I tried running the tests from the file The change we have in the version plots2/app/views/map/map.html.erb Line 104 in fbabe28
|
Hmm. i started by lengthening to 180, just to see. But i also see these timeouts which affect tests that aren't the map system test... Just noting here before we think about the next step.
(ref https://travis-ci.org/publiclab/plots2/jobs/640065339#L3773) |
actually @sagarpreet-chadha while we're working this problem, i wonder if you could aim to get a system test built that can confirm that the output of some especially critical layers are present on maps. Even just a javascript assertion about the layer contents plus a screenshot of a number of layers showing would be great. cc #5316 |
It looks almost like it's hanging and then it just keeps on counting and not doing anything. The numbers keep getting bigger down that test list. |
Is it possible that it's looking for something that it's not finding? @VladimirMikulic you had some tips on system tests getting stuck; did you maybe have any insight here? Thank you! |
@jywarren it can be the problem. I would rather refactor this to be: map_center = page.evaluate_script("Math.round(map.getCenter().lng)")
assert_equal(-13, map_center) This would be applied to every test. If map. This should resolve the problem. |
You may ask "Then why these tests were working in the past?". Thank you for asking for my opinion though, maybe I could set up a branch to test this? |
Ah ok! So this is what we would use for the tests:
|
@nstjean 👍 Just one note. Before assertion, I would call |
Ahh I see now.
@jywarren Do you want to add this code? or do you want me to push up the changes to my own branch? |
Would one of you be able to build on this branch but open a new PR including this code and the suggested modifications? Thank you all so much. I'm thinking about how we can make some system test "tips and guidelines" that help us keep best practices on this. Maybe a part of the testing README? And, we could potentially have a bot respond with tips if anything in the system tests folder changes? 🙌 🎉 |
I can build the changes 👍 |
OMG thanks a ton. Had a really busy day and it was wonderful to have support on this issue in parallel. Much appreciated 🙌 |
@jywarren no problem. What I've found out is that maps are not loading at all. |
@VladimirMikulic That's interesting... Can we find out why? I assusme maps on other pages are loading properly since they are passing tests? |
Can we add a screenshot at the end, just as one more way to assess the map loading? |
@nstjean I can't confirm that. On localhost, they all work, but in Travis, for some reason, they are not loading. @jywarren unfortunately, we can't add screenshots. |
What should we do then? Should we remove those 3 tests so we can get LEL bumped, then try to figure it out later? |
Commenting out/removing tests is risky. Travis is building this for production, tests are telling us that maps are not loading. They probably won't load in production. I could compare the two versions of LEL and try to identify why the 2.1.9 is not working. |
Ok, we'll let me know if there's anything I can do to help.
…On Thu, Jan 23, 2020, 4:24 PM Vladimir Mikulic ***@***.***> wrote:
Commenting out/removing tests is risky. Travis is building this for
production, tests are telling us that maps are not loading. They probably
won't load in production.
I could compare the two versions of LEL and try to identify why the 2.1.9
is not working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7299?email_source=notifications&email_token=ALZLKMN4ZKXZGZMBQKFQYRDQ7IDIXA5CNFSM4KIWDRR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJY45GA#issuecomment-577883800>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALZLKMK4RGR5YN7VWOD2FOTQ7IDIXANCNFSM4KIWDRRQ>
.
|
@nstjean your experience with LEL could certainly help. |
@crisner has significantly more experience with LEL, she would probably know better than I, but tomorrow I'll take a look at what code was changed to see if I can spot anything! |
Thanks to @VladimirMikulic I think I figured out what could be causing the issue. The two bug fixes were:
I have made a PR #7340 with changes that I tested on travis with my forked repo. I think the main issue was the base layer was being added twice. I have now passed in the baselayer to the one-liner code so that this doesn't happen. When I tested this branch in my forked plots2 repo, all tests seem to pass. |
The tests are passing on the PR I made. https://travis-ci.org/publiclab/plots2/builds/641207513?utm_source=github_status&utm_medium=notification In the current change, the map page will have all the layers on the layers control(only a few layers were available in the last commit) and the layers will not be displayed by default. When I tried to include layers in the oneliner code I got the timeout error as seen here: https://travis-ci.org/crisner/plots2/jobs/641199584?utm_medium=notification&utm_source=github_status We could perhaps increase the wait time again to check if it would work. |
Awesome work @crisner! I am glad I was able to help. |
Hooray!! |
So right now no layers are being automatically added to the maps? We will need that functionality down the road, as I'm going to be adding the PLpeople layer to the /maps/ page. And I know that layer is currently really slow. |
The layers will not be displayed automatically only on the /map page. Adding back the include option will display the maps automatically. In the next version of LEL we will need to pass in an option to the LEL instantiation if we need to display the maps automatically on page load. |
Im working on the documentation right now so that it is easier to refer information on LEL's usage. |
Ok! I'll definitely be using that documentation, I'm trying to get a handle on how it all works together. :) |
Solved in #7340 with BIG help from @VladimirMikulic and @crisner !!! Great teamwork!!! |
Hi @crisner - just double checking, for inline maps short codes, do we still support showing maps by default there? Such as in: https://publiclab.org/wiki/inline-maps#Maps+with+preset+layers Thanks! |
@crisner I think i answered my own question by looking at https://stable.publiclab.org/wiki/inline-maps#Maps+with+preset+layers -- yes, those layers are shown by default, BEAUTIFULLY! Thank you!! |
Yes! That looks great!! |
🚀 🚀 🚀 |
Supercede #7272