version bump LEL 2.1.9 #7299

jywarren · 2020-01-19T00:32:29Z

Supercede #7272

jywarren · 2020-01-19T00:34:29Z

codecov · 2020-01-19T00:40:41Z

Codecov Report

Merging #7299 into master will increase coverage by 1.94%.
The diff coverage is 100%.

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

Impacted Files	Coverage Δ
app/models/user.rb	`91.05% <100%> (+4.33%)`	⬆️
app/models/comment.rb	`76.95% <100%> (+6.95%)`	⬆️
app/controllers/user_sessions_controller.rb	`66.45% <100%> (ø)`	⬆️
app/controllers/users_controller.rb	`81.05% <100%> (-0.64%)`	⬇️
app/channels/application_cable/connection.rb	`0% <0%> (-100%)`	⬇️
app/channels/application_cable/channel.rb	`0% <0%> (-100%)`	⬇️
app/channels/user_notification_channel.rb	`0% <0%> (-83.34%)`	⬇️
app/channels/user_channel.rb	`0% <0%> (-83.34%)`	⬇️
app/channels/room_channel.rb	`0% <0%> (-71.43%)`	⬇️
app/controllers/questions_controller.rb	`81.57% <0%> (-9.22%)`	⬇️
... and 19 more

crisner · 2020-01-19T04:22:54Z

Weird. The failing tests are,

ERROR["test_correct_url_hash_for_wiki_map", #<Minitest::Reporters::Suite:0x00007f55f8bd7db0 @name="MapTest">, 403.47831010299996]
 test_correct_url_hash_for_wiki_map#MapTest (403.48s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/map_test.rb:9:in `block in <class:MapTest>'
ERROR["test_show_map_by_hash_location", #<Minitest::Reporters::Suite:0x0000555679c2ef78 @name="MapTest">, 703.716054941]
 test_show_map_by_hash_location#MapTest (703.72s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/map_test.rb:17:in `block in <class:MapTest>'
ERROR["test_url_hash_updates_when_map_panned", #<Minitest::Reporters::Suite:0x0000555677886fd0 @name="MapTest">, 1004.005064855]
 test_url_hash_updates_when_map_panned#MapTest (1004.01s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/map_test.rb:28:in `block in <class:MapTest>'

I checked this by bumping LEL locally. I couldn't check the wiki page but the other two tests should pass. Here's the gif:

VladimirMikulic · 2020-01-19T04:36:42Z

We are using Chrome driver version 74, right?
My first solution would be to update to the latest one.
If we can not do that we should try adding this in application_system_test_case.rb file:

chromeOptions = %w(--headless --disable-gpu --no-sandbox --remote-debugging-port=9222 --enable-features=NetworkService,NetworkServiceInProcess)

nstjean · 2020-01-20T15:20:24Z

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

Capybara.default_max_wait_time = 90

jywarren · 2020-01-21T17:48:43Z

OK, thanks so much! Tried that, let's see!

jywarren · 2020-01-21T19:31:17Z

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?

nstjean · 2020-01-21T19:42:16Z

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 @layersname variable with an empty array so that it doesn't add LEL layers to the map?

jywarren · 2020-01-21T21:27:04Z

Hm, i'll give it a try - would you mind pointing me to the specific line of code? Thank you!!

nstjean · 2020-01-21T22:01:24Z

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

    
           <% if @layersname.nil? %> 
        
             var layersname = ['odorreport', 'asian', 'clouds', 'eonetFiresLayer', 'Unearthing']; 
        
           <% else %> 
        
             var layersname = "<%= @layersname %>"; 
        
             layersname = layersname.split(',') 
        
           <% end %>

I played with it on my computer and setting the default to no layers didn't speed up the tests at all, the whole map_test.rb file runs in 6.02 seconds. If something on the testing server is making it take more than 120 seconds we're going to have to figure out what is causing it.

crisner · 2020-01-22T06:32:03Z

I tried running the tests from the file map_test.rb on my machine and it takes 13 seconds for the tests to complete. With layers turned off it takes 9 seconds to complete.

The change we have in the version 2.1.9 of LEL is displaying the layers by default on the map when include option is true. This seems to be taking an extra 4 seconds when running tests. This PR#353 has included the feature to turn it off. For now, we can stop the layers from being added to the map by commenting out this line:

plots2/app/views/map/map.html.erb

Line 104 in fbabe28

include: layersname,

jywarren · 2020-01-22T16:24:29Z

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.

ERROR["test_correct_url_hash_for_wiki_map", #<Minitest::Reporters::Suite:0x00007f162a4c03c8 @name="MapTest">, 381.746383889]
 test_correct_url_hash_for_wiki_map#MapTest (381.75s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/map_test.rb:9:in `block in <class:MapTest>'
ERROR["test_show_map_by_hash_location", #<Minitest::Reporters::Suite:0x00007f162a6eae28 @name="MapTest">, 682.0204063890001]
 test_show_map_by_hash_location#MapTest (682.02s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/map_test.rb:17:in `block in <class:MapTest>'
ERROR["test_url_hash_updates_when_map_panned", #<Minitest::Reporters::Suite:0x00007f162a91ab30 @name="MapTest">, 982.2769954369999]
 test_url_hash_updates_when_map_panned#MapTest (982.28s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/map_test.rb:28:in `block in <class:MapTest>'
ERROR["test_simple-data-grapher_powertag", #<Minitest::Reporters::Suite:0x00007f162a397a28 @name="CsvfilesControllersTest">, 1282.5615456599999]
 test_simple-data-grapher_powertag#CsvfilesControllersTest (1282.56s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/csvfiles_controllers_test.rb:7:in `block in <class:CsvfilesControllersTest>'
ERROR["test_visit_simple-data-grapher", #<Minitest::Reporters::Suite:0x00007f162ae66fd0 @name="CsvfilesControllersTest">, 1582.856890126]
 test_visit_simple-data-grapher#CsvfilesControllersTest (1582.86s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/csvfiles_controllers_test.rb:13:in `block in <class:CsvfilesControllersTest>'
ERROR["test_adding_a_tag_to_a_user_profile", #<Minitest::Reporters::Suite:0x00007f162b096260 @name="TagTest">, 1883.079794101]
 test_adding_a_tag_to_a_user_profile#TagTest (1883.08s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/tag_test.rb:9:in `setup'
ERROR["test_adding_a_tag_via_javascript", #<Minitest::Reporters::Suite:0x00007f162b41b480 @name="TagTest">, 2183.3668210289998]
 test_adding_a_tag_via_javascript#TagTest (2183.37s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/tag_test.rb:9:in `setup'
ERROR["test_adding_a_tag_via_javascript_with_url_only", #<Minitest::Reporters::Suite:0x00007f162b75c360 @name="TagTest">, 2483.6060915049998]
 test_adding_a_tag_via_javascript_with_url_only#TagTest (2483.61s)
Minitest::UnexpectedError:         Net::ReadTimeout: Net::ReadTimeout
            test/system/tag_test.rb:9:in `setup'

(ref https://travis-ci.org/publiclab/plots2/jobs/640065339#L3773)

jywarren · 2020-01-22T16:30:51Z

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

nstjean · 2020-01-22T17:18:44Z

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.

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.

jywarren · 2020-01-22T19:07:52Z

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!

VladimirMikulic · 2020-01-22T19:33:20Z

@jywarren it can be the problem.
The thing I've noticed with mapt_test.rb tests is that they are doing what I call "dynamic assertion".
assert_equal(-13, page.evaluate_script("Math.round(map.getCenter().lng)"))
As you've mentioned we've already enountered problems with this approach.

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.methods behind the scenes use JavaScript's task queue (I mean asynchronous operations like Promises, intervals...) I would add wait_for_ajax method to wait until the method completes.

This should resolve the problem.

VladimirMikulic · 2020-01-22T19:36:53Z

You may ask "Then why these tests were working in the past?".
Well, the answer to this would be a little more complicated.
The code is changing and a lot of code has been added recently, sometimes it can cause delays and make tests time out. That's why it's hard to debug these types of errors.

Thank you for asking for my opinion though, maybe I could set up a branch to test this?

nstjean · 2020-01-22T19:38:21Z

Ah ok! So this is what we would use for the tests:


  test 'correct url hash for wiki map' do
    visit '/map/chicago'

    # check that the url hash is correct
    hash = page.evaluate_script("window.location.hash")
    assert_equal("#13/41.87/-87.64", hash)
    
  end

  test 'show map by hash location' do
    visit '/map#9/-25/-13'

    lat = page.evaluate_script("Math.round(map.getCenter().lat)")
    lng = page.evaluate_script("Math.round(map.getCenter().lng)")
    zoom = page.evaluate_script("map.getZoom()")
    assert_equal(-25, lat)
    assert_equal(-13, lng)
    assert_equal(9, zoom)

  end

  test 'url hash updates when map panned' do
    visit '/map'

    page.execute_script("map.setView([13, 60], 15)")

    # check that the url hash is correct
    hash = page.evaluate_script("window.location.hash")
    assert_equal("#15/13/60", hash)
    
  end

VladimirMikulic · 2020-01-22T19:42:28Z

@nstjean 👍

Just one note. Before assertion, I would call wait_for_ajax funtion to wait in case the map method uses an asynchronous operation which I am sure it does (ajax request for example).

nstjean · 2020-01-22T19:46:56Z

Ahh I see now.


  test 'correct url hash for wiki map' do
    visit '/map/chicago'

    # check that the url hash is correct
    hash = page.evaluate_script("window.location.hash")
    wait_for_ajax
    assert_equal("#13/41.87/-87.64", hash)
    
  end

  test 'show map by hash location' do
    visit '/map#9/-25/-13'

    lat = page.evaluate_script("Math.round(map.getCenter().lat)")
    lng = page.evaluate_script("Math.round(map.getCenter().lng)")
    zoom = page.evaluate_script("map.getZoom()")
    wait_for_ajax
    assert_equal(-25, lat)
    assert_equal(-13, lng)
    assert_equal(9, zoom)

  end

  test 'url hash updates when map panned' do
    visit '/map'

    page.execute_script("map.setView([13, 60], 15)")

    # check that the url hash is correct
    hash = page.evaluate_script("window.location.hash")
    wait_for_ajax
    assert_equal("#15/13/60", hash)
    
  end

@jywarren Do you want to add this code? or do you want me to push up the changes to my own branch?

jywarren · 2020-01-22T20:33:47Z

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?

🙌 🎉

VladimirMikulic · 2020-01-22T21:00:10Z

I can build the changes 👍
System tests are really special types of tests and making system tests tips would certainly be useful.
I can do it as well.

jywarren · 2020-01-22T21:49:39Z

OMG thanks a ton. Had a really busy day and it was wonderful to have support on this issue in parallel. Much appreciated 🙌

VladimirMikulic · 2020-01-22T21:50:18Z

@jywarren no problem. What I've found out is that maps are not loading at all.
They load locally, do you have any idea why in Travis they don't load?

nstjean · 2020-01-23T00:34:57Z

@VladimirMikulic That's interesting... Can we find out why? I assusme maps on other pages are loading properly since they are passing tests?

jywarren · 2020-01-23T18:05:21Z

Can we add a screenshot at the end, just as one more way to assess the map loading?

VladimirMikulic · 2020-01-23T19:15:54Z

@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.
The visit statement fails(it's the first statement in every test) and other lines are not getting executed, so placing take_screenshot below it wouldn't actually generate a screenshot.

nstjean · 2020-01-23T20:44:19Z

What should we do then? Should we remove those 3 tests so we can get LEL bumped, then try to figure it out later?

VladimirMikulic · 2020-01-23T21:24:26Z

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.

nstjean · 2020-01-23T21:26:35Z

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

VladimirMikulic · 2020-01-23T21:40:24Z

@nstjean your experience with LEL could certainly help.
Do you have any idea why maps are not loading in 2.1.9?
It's not a breaking change, just bug fixes.

nstjean · 2020-01-24T01:11:26Z

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

crisner · 2020-01-24T04:36:36Z

Thanks to @VladimirMikulic I think I figured out what could be causing the issue. The two bug fixes were:

fixing error thrown in the console because there was no basemap in the one-liner code
Automatically displaying layers on page load

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.
Displaying the layers by default could be causing issues too as it takes longer to load on the map. I have commented out the include option responsible for this. I have set the wait time back to 90. This may work if the wait time is increased.

When I tested this branch in my forked plots2 repo, all tests seem to pass.
https://travis-ci.org/crisner/plots2/jobs/641203838

crisner · 2020-01-24T04:48:16Z

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.

VladimirMikulic · 2020-01-24T09:28:07Z

Awesome work @crisner! I am glad I was able to help.
If maps need time to load, then we should increase the wait time.

nstjean · 2020-01-24T13:56:49Z

Hooray!!

nstjean · 2020-01-24T14:00:28Z

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.

crisner · 2020-01-24T14:11:01Z

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.

crisner · 2020-01-24T14:13:13Z

Im working on the documentation right now so that it is easier to refer information on LEL's usage.

nstjean · 2020-01-24T14:19:05Z

Ok! I'll definitely be using that documentation, I'm trying to get a handle on how it all works together. :)

jywarren · 2020-01-24T15:40:34Z

Solved in #7340 with BIG help from @VladimirMikulic and @crisner !!! Great teamwork!!!

jywarren · 2020-01-24T15:42:00Z

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!

jywarren · 2020-01-24T15:47:27Z

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

nstjean · 2020-01-24T15:52:06Z

Yes! That looks great!!

VladimirMikulic · 2020-01-24T16:33:47Z

🚀 🚀 🚀

version bump LEL

cc1d50c

jywarren mentioned this pull request Jan 19, 2020

Bump leaflet-environmental-layers from 2.1.7 to 2.1.9 #7272

Closed

Update map_test.rb

11eaf8b

wait time to 180

4d93555

VladimirMikulic mentioned this pull request Jan 22, 2020

FIX: map_test.rb system tests Travis error while upgrading to LEL 2.1.9 #7330

Closed

crisner mentioned this pull request Jan 24, 2020

Fix build errors on lel version bump to 2.1.9 #7340

Merged

jywarren mentioned this pull request Jan 24, 2020

Map markers for nodes only appear after map interaction, not on initial load #6910

Closed

jywarren closed this Jan 24, 2020

version bump LEL 2.1.9 #7299

version bump LEL 2.1.9 #7299

Conversation

jywarren commented Jan 19, 2020

jywarren commented Jan 19, 2020

codecov bot commented Jan 19, 2020 • edited Loading

Codecov Report

crisner commented Jan 19, 2020

VladimirMikulic commented Jan 19, 2020 • edited Loading

nstjean commented Jan 20, 2020

jywarren commented Jan 21, 2020

jywarren commented Jan 21, 2020

nstjean commented Jan 21, 2020

jywarren commented Jan 21, 2020

nstjean commented Jan 21, 2020

crisner commented Jan 22, 2020

jywarren commented Jan 22, 2020 • edited Loading

jywarren commented Jan 22, 2020

nstjean commented Jan 22, 2020

jywarren commented Jan 22, 2020

VladimirMikulic commented Jan 22, 2020 • edited Loading

VladimirMikulic commented Jan 22, 2020 • edited Loading

nstjean commented Jan 22, 2020

VladimirMikulic commented Jan 22, 2020 • edited Loading

nstjean commented Jan 22, 2020

jywarren commented Jan 22, 2020

VladimirMikulic commented Jan 22, 2020

jywarren commented Jan 22, 2020

VladimirMikulic commented Jan 22, 2020 • edited Loading

nstjean commented Jan 23, 2020

jywarren commented Jan 23, 2020

VladimirMikulic commented Jan 23, 2020

nstjean commented Jan 23, 2020

VladimirMikulic commented Jan 23, 2020

nstjean commented Jan 23, 2020 via email

VladimirMikulic commented Jan 23, 2020

nstjean commented Jan 24, 2020

crisner commented Jan 24, 2020

crisner commented Jan 24, 2020

VladimirMikulic commented Jan 24, 2020

nstjean commented Jan 24, 2020

nstjean commented Jan 24, 2020

crisner commented Jan 24, 2020

crisner commented Jan 24, 2020

nstjean commented Jan 24, 2020

jywarren commented Jan 24, 2020

jywarren commented Jan 24, 2020

jywarren commented Jan 24, 2020

nstjean commented Jan 24, 2020

VladimirMikulic commented Jan 24, 2020

codecov bot commented Jan 19, 2020 •

edited

Loading

VladimirMikulic commented Jan 19, 2020 •

edited

Loading

jywarren commented Jan 22, 2020 •

edited

Loading

VladimirMikulic commented Jan 22, 2020 •

edited

Loading

VladimirMikulic commented Jan 22, 2020 •

edited

Loading

VladimirMikulic commented Jan 22, 2020 •

edited

Loading

VladimirMikulic commented Jan 22, 2020 •

edited

Loading