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

version bump LEL 2.1.9 #7299

Closed
wants to merge 3 commits into from
Closed

version bump LEL 2.1.9 #7299

wants to merge 3 commits into from

Conversation

jywarren
Copy link
Member

Supercede #7272

@jywarren
Copy link
Member Author

cc @crisner @nstjean @VladimirMikulic !

@codecov
Copy link

codecov bot commented Jan 19, 2020

Codecov Report

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

Impacted file tree graph

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

crisner commented Jan 19, 2020

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:
urltest

@VladimirMikulic
Copy link
Contributor

VladimirMikulic commented Jan 19, 2020

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

nstjean commented Jan 20, 2020

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.

Capybara.default_max_wait_time = 90

@jywarren
Copy link
Member Author

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

@jywarren
Copy link
Member Author

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

nstjean commented Jan 21, 2020

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

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

@nstjean
Copy link
Contributor

nstjean commented Jan 21, 2020

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!

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

crisner commented Jan 22, 2020

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:

include: layersname,

@jywarren
Copy link
Member Author

jywarren commented Jan 22, 2020

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

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

nstjean commented Jan 22, 2020

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

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

VladimirMikulic commented Jan 22, 2020

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

VladimirMikulic commented Jan 22, 2020

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

nstjean commented Jan 22, 2020

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

VladimirMikulic commented Jan 22, 2020

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

nstjean commented Jan 22, 2020

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

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

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

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

@VladimirMikulic
Copy link
Contributor

VladimirMikulic commented Jan 22, 2020

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

nstjean commented Jan 23, 2020

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

@jywarren
Copy link
Member Author

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

@VladimirMikulic
Copy link
Contributor

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

nstjean commented Jan 23, 2020

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

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

nstjean commented Jan 23, 2020 via email

@VladimirMikulic
Copy link
Contributor

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

nstjean commented Jan 24, 2020

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

crisner commented Jan 24, 2020

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

crisner commented Jan 24, 2020

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

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

nstjean commented Jan 24, 2020

Hooray!!

@nstjean
Copy link
Contributor

nstjean commented Jan 24, 2020

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

crisner commented Jan 24, 2020

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

crisner commented Jan 24, 2020

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

@nstjean
Copy link
Contributor

nstjean commented Jan 24, 2020

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

@jywarren
Copy link
Member Author

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

@jywarren jywarren closed this Jan 24, 2020
@jywarren
Copy link
Member Author

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

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

nstjean commented Jan 24, 2020

Yes! That looks great!!

@VladimirMikulic
Copy link
Contributor

🚀 🚀 🚀

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.

4 participants