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

write tests that assert the markers are shown or hidden as in the new example code #52

Merged
merged 15 commits into from
Apr 2, 2019

Conversation

sagarpreet-chadha
Copy link
Collaborator

Fixes #45 !

@sagarpreet-chadha
Copy link
Collaborator Author

Hi @jywarren !

dist/Leaflet.BlurredLocationDisplay.js is not being loaded before the tests are loaded . Can you help in setting up the testing environment ? Thanks !

screenshot 2019-03-08 at 5 03 21 pm

@sagarpreet-chadha
Copy link
Collaborator Author

Hi @jywarren ,
Kindly take a look at it once 😄 !

Gruntfile.js Outdated
'node_modules/jasmine-ajax/lib/mock-ajax.js',
'https://maps.googleapis.com/maps/api/js?libraries=places&language=en&key=AIzaSyDWgc7p4WWFsO3y0MTe50vF4l4NUPcPuwE',
'node_modules/leaflet/dist/leaflet-src.js',
'node_modules/leaflet-blurred-location/dist/Leaflet.BlurredLocation.js' ,
Copy link
Member

Choose a reason for hiding this comment

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

<link href="../../../node_modules/leaflet/dist/leaflet.css" rel="stylesheet">
<script src="../../../node_modules/leaflet/dist/leaflet.js"></script>
<script src="../../../node_modules/leaflet-blurred-location/dist/Leaflet.BlurredLocation.js"></script>
<script src="../../../dist/Leaflet.BlurredLocationDisplay.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

So i think you may want to load these in the Gruntfile on these lines: https://github.com/publiclab/leaflet-blurred-location-display/pull/52/files#diff-35b4a816e0441e6a375cd925af50752cR35

because they won't be loaded from this file until you load the fixture. And some of the tests you may not actually want the HTML fixture data, but just to test basic methods in the JS lib.

See for example:

https://github.com/publiclab/leaflet-blurred-location/blob/4f452d5cc486ac320fa168eeabbb46bedb1b8378/spec/javascripts/fixtures/index.html#L5-L8

@sagarpreet-chadha
Copy link
Collaborator Author

Yes tried that as well !
Added LBL , LBLD dependencies in vendor , still showing the same error .
LBLD is not getting loaded before the specs starts running .

@jywarren
Copy link
Member

Hmm, confusing. Maybe you have to include the source files like this?

https://github.com/publiclab/PublicLab.Editor/blob/master/Gruntfile.js#L44-L46

If this doesn't work, maybe ping @rexagod or someone else from image-sequencer as they are really good with test setup!

@sagarpreet-chadha
Copy link
Collaborator Author

Hey @rexagod , @tech4GT , @publiclab/reviewers !
Can someone with good testing background help me solve this one ? Looking forward to hear from you guys 😄 . Thank you :)

@rexagod
Copy link
Member

rexagod commented Mar 21, 2019

Noticing that Travis and local both give different errors (BlurredLocation and BlurredLocationDisplay respectively). @sagarpreet-chadha Are you getting an asynchronous warning in your logs as well?

Working on this.


it("Checks if at zoom level 5 , all 7 markers are shown", function () {
//BlurredLocation1.map.setZoom(5);
expect($("#map1").children()[0].childNodes[3].childNodes.length).toBe(0) ;
Copy link
Member

Choose a reason for hiding this comment

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

expect($("#map1").children()[0].childNodes[3].childNodes.length).toBe(7) ;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @rexagod , yes i did that purposely to tell you guys that in testing , markers are not getting loaded on map 😄 !


it("Checks if at zoom level 6 , only 5 markers are shown", function () {
//BlurredLocation2.map.setZoom(6);
expect($("#map2").children()[0].childNodes[3].childNodes.length).toBe(0) ;
Copy link
Member

Choose a reason for hiding this comment

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

expect($("#map1").children()[0].childNodes[3].childNodes.length).toBe(5) ;


it("Checks if at zoom level 8 , only 3 markers are shown", function () {
// BlurredLocation3.map.setZoom(8);
expect($("#map3").children()[0].childNodes[3].childNodes.length).toBe(0) ;
Copy link
Member

Choose a reason for hiding this comment

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

expect($("#map1").children()[0].childNodes[3].childNodes.length).toBe(3) ;

@rexagod
Copy link
Member

rexagod commented Mar 23, 2019

Hello, @sagarpreet-chadha! I've made some comments above, depending on some checks I made below.

Screenshot from 2019-03-22 23-51-19

PS. All the tests are passing if length is expected to be 0, meaning that for some reason the DOMchildren aren't being loaded.

@rexagod
Copy link
Member

rexagod commented Mar 23, 2019

Getting contradicting results.

Screenshot from 2019-03-23 17-43-01

Screenshot from 2019-03-23 17-42-47

@rexagod
Copy link
Member

rexagod commented Mar 23, 2019

Made some changes along the way.

if(map.getZoom() >= 3 && map.getZoom() <=9){

  • Changed to if(map && map.getZoom() >= 3 && map.getZoom() <=9), since getZoom wasn't initialized onDOMload so we might want to look into that as well.

if(typeof options.blurredLocation.getRectangle() !== "undefined"){

  • if(typeof options.blurredLocation.getRectangle !== "undefined"), removed parenthesis from above, since getRectangle itself wasn't on blurredLocation's chain.

@sagarpreet-chadha
Copy link
Collaborator Author

Hi @rexagod ...thank you for the recommendations 😄 . What do you think could be the possible reason that LBL and LBLD libraries are not getting loaded before testing starts ? Thanks !

@sagarpreet-chadha
Copy link
Collaborator Author

Thank you @arpansac , the fixtures are now being loaded 🎉 .
2 out of 5 tests are running now 👍 .

Also now all libraries are also getting loaded except LBLD .
Screenshot 2019-03-28 at 10 52 33 PM

Now we just have to figure why LBLD is not getting loaded . Thanks all 😄 !

@sagarpreet-chadha
Copy link
Collaborator Author

We use Jasmine jquery to load fixtures , but this package is not maintained . Also see this which i found in its documentation (this should solve the error which one gets when grunt jasmine is run on localhost) :
Source : https://github.com/velesin/jasmine-jquery
Screenshot 2019-04-01 at 3 32 08 PM

And this error on Travis 😄 asks to open issue in puppeteer package :

Screenshot 2019-04-01 at 3 43 04 PM

@sagarpreet-chadha
Copy link
Collaborator Author

grunt-contrib-Jasmine runs on phantomJS , and phantomJS does not support ES6 !

@sagarpreet-chadha
Copy link
Collaborator Author

YAYYY 🎉

I think i will write a blog about it :P .
So some ES6 functions were causing problem because phantomJS does not support ES6 !
And regarding LBLD not loading in fixtures --- it is yet an unsolved issue in Jasmine-jquery !

@jywarren ...kindly review 😄 !
Should we add put this library to npm ?

@sagarpreet-chadha
Copy link
Collaborator Author

@jywarren ...kindly review this one as well 😄 !

@jywarren
Copy link
Member

jywarren commented Apr 2, 2019

OK! Sorry i had replied via email but it didn't go through!

@jywarren jywarren merged commit ab0774f into main Apr 2, 2019
@jywarren
Copy link
Member

jywarren commented Apr 2, 2019

Awesome! Thanks Sagarpreet and great problem solving, this was such a tough one. I'm sorry i wasn't able to help more! Perhaps a good deal of this testing workflow is starting to age a bit. But you got it! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants