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.

Sign up for GitHub

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

Jump to bottom

[WIP] Test indigeneous layers #222

Merged
merged 8 commits into from
Aug 19, 2019
Merged

[WIP] Test indigeneous layers #222

merged 8 commits into from
Aug 19, 2019

Conversation

ananyaarun
Copy link
Member

@ananyaarun ananyaarun commented Jul 3, 2019 •
edited
Loading

Fixes #223

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

@ananyaarun
Copy link
Member Author

@sagarpreet-chadha @rexagod @jywarren , this is simple test to load the fixture and test.
Actually indigenous layers dont have the get markers and add markers methods so what exactly can we test for ? The polygons rendered ?

Also can someone pls explain this line of code

expect($("#map").children()[0].childNodes[2].childNodes[1].childNodes[0].childNodes.length).toBeGreaterThan(10) ;

Thanks!!

@sagarpreet-chadha
Copy link
Collaborator

Okay so this piece of code basically tests the presence of rectangles on the PLpeople layer and it should be more than lets say 10 (maybe more than 100 depending on the zoom level) .
We can see the HTML code of marker by viewing the console code on browser and using children() method we reach the marker div .

Similarly you can test the presence of Polygon in the indigineous layers . Thanks

@ananyaarun
Copy link
Member Author

Hey @sagarpreet-chadha is the PLpeople layer fixture file working ?
I got these errors while I was trying to figure out the markers div.

error

The LBLD scrips are not loading due to which the PLpeople layer fixture doesn't work as it needs this script

@ananyaarun
Copy link
Member Author

Hey @sagarpreet-chadha I see in some PR the changes you made in index.html for PLpeoples layer has been removed by mistake in the master branch. I guess this happened during index.html refactoring...
I'll fix this.
The above error would also be resolved by this I guess.

@ananyaarun ananyaarun mentioned this pull request Jul 4, 2019
Merged
5 tasks
@ananyaarun
Copy link
Member Author

Hey @sagarpreet-chadha @rexagod , I have written tests for these layers but while testing them locally I face these issues.
Here is a screenshot of the PLpeople Layer

error

I have followed a similar approach for the indigenous layers but it doesn't seem to work locally.

@ananyaarun
Copy link
Member Author

Why is load fixture not defined ?
It is a jasmine jquery inbuild function right ?

@rexagod
Copy link
Member

rexagod commented Jul 6, 2019

Seems like a jasmine-jquery preload issue to me. Are you loading this through grunt?

@ananyaarun
Copy link
Member Author

ananyaarun commented Jul 6, 2019 •
edited
Loading

Hey @rexagod , no I am not using grunt to load this.
I installed jasmine jquery and basically used this to locally run my tests
https://jasmine.github.io/setup/nodejs.html

@rexagod
Copy link
Member

rexagod commented Jul 7, 2019

It seems jasmine-jQuery should be included last, or after all jasmine scripts have been loaded. Obvious, but worth trying. Let me know if this fixes it.

https://stackoverflow.com/q/24531674

sagarpreet-chadha
sagarpreet-chadha reviewed Jul 7, 2019
View reviewed changes
Copy link
Collaborator

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Hey @ananyaarun ,
First try the possible solution by @rexagod , then do take a look at this PR (publiclab/leaflet-blurred-location-display#52) where i faced this exactly same issue and found a work way around 😄 !

@jywarren
Copy link
Member

jywarren commented Jul 8, 2019 via email

@ananyaarun
Copy link
Member Author

Hey Thanks everyone !!
I got the tests running now locally.
the tests for PLpeople layer written previously pass, I'm still trying to figure out why indigenous layer tests are failing.

@sagarpreet-chadha
Copy link
Collaborator

Hi @ananyaarun , what is the progress here? Do you need help here ?

@ananyaarun
Copy link
Member Author

ananyaarun commented Jul 9, 2019 •
edited
Loading

Hey @sagarpreet-chadha yes,

So I have written tests for the 3 indigenous land layers which I suppose are correct (as per the html code I see once the polygons load).

The thing is once you choose these layers, the polygons don't appear instantly like for example PLpeoples layer where grids appear instantly, and the childnode divs are only created after zooming in multiple times after a while.

Hence the tests I have written fail , I am trying a lot of trial and error with the zoom values to get them to pass but they are failing atm.
What do you think I can do here ?

@ananyaarun
Copy link
Member Author

Here @jywarren , this is the PR for writing tests

@jywarren
Copy link
Member

jywarren commented Jul 9, 2019 via email

@ananyaarun
Copy link
Member Author

ananyaarun commented Jul 10, 2019 •
edited
Loading

Sure @jywarren I'll give that and what @sagarpreet-chadha said a try :)
Thanks!
This is taking me a while to figure out actually, hence I am parallely working on standardization.

@ananyaarun
Copy link
Member Author

Hey @sagarpreet-chadha Im really stuck on this one.
I havent been able to get the tests to pass till now.

Also you said something on the call the other day about loading only one marker in the fixture file....
but in the PLpeople layer i see this hasn't been done.
I just see the blurred location function and the PLpeople layer function.

Can you pls review this once if possible.
I just want to make sure my approach is correct.
Thanks!

sagarpreet-chadha
sagarpreet-chadha reviewed Jul 29, 2019
View reviewed changes
spec/javascripts/indigenousLanguagesLayers_spec.js Outdated

it("Checks if layer polygons are present", function () {
// There should be atleast one polygon in the Language layer always .
expect($("#map").children()[0].childNodes[2].childNodes[0].childNodes[0].childNodes.length).toBeGreaterThan(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi...this is correct testing .
But you have to understand that these rectangles are made from a remote API, hence it takes time for these rectangles to appear on page, right?

So there are two things you should try in the following order:

  1. Try to preempt the execution of test till the rectangles appear on website .
    https://jasmine.github.io/tutorials/async

  2. If the asynchronous approach of testing does not works for some reason, what we can do is to look at what we have to actually test i.e the layer should be loaded when the respective function is called , right?
    So what we can do is basically in the fixture add a L.polygon object on the map and test if it occurs on map .
    In this way atleast we can test that the layer is being loaded or not . Later we can test if the actual data is getting loaded from remote API or not . Makes sense?

cc @jywarren

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks,
I'll give this another try and get back to you.

Copy link
Member Author

@ananyaarun ananyaarun Jul 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The async methods don't seem to work.
I'll try the 2nd method @sagarpreet-chadha

@ananyaarun
Copy link
Member Author

ananyaarun commented Aug 3, 2019 •
edited
Loading

Hey @sagarpreet-chadha @rexagod
I am pretty sure my tests are correct now and I have used the second method for testing.
I have added a polygon abject and I am testing.
There is a problem I am facing 3 days now which I really don't know how to go about.
My tests still fail!!

error

I wrote a very basic test like this just to test and that is failing too I really don't know why.


  it("Checks if layer polygons are present", function () {
  expect($("#map").children()[0].childNodes.length).toBeGreaterThan(-1) ;
  });

PS: Over here i am just checking if it is greater than -1
so that even if for some reason the L.polygon method is not working tests still pass but the problem seems to be with an undefined object now and they still fail.

Map is the div id whose child node we are accessing. Same is done in the PLpeople layer tests.
I really do not understand the undefined object error.

Please let me know if You figure out, Thanks

@sagarpreet-chadha
Copy link
Collaborator

sagarpreet-chadha commented Aug 3, 2019 via email

@ananyaarun
Copy link
Member Author

Sure Thanks @sagarpreet-chadha :)
Can we start discussion on this issue ?
#175
Or is there other issues that are priority ?

@sagarpreet-chadha
Copy link
Collaborator

sagarpreet-chadha commented Aug 11, 2019 via email

@ananyaarun
Copy link
Member Author

@sagarpreet-chadha @rexagod can we discuss this in todays open call ?

@sagarpreet-chadha
Copy link
Collaborator

sagarpreet-chadha commented Aug 13, 2019 via email

@sagarpreet-chadha
Copy link
Collaborator

@jywarren , i think we should change our testing framework if we need to test properly . Jasmine is giving a lot of issues .
Ping me when we should discuss this . Thanks!

@jywarren
Copy link
Member

jywarren commented Aug 14, 2019 via email

@sagarpreet-chadha
Copy link
Collaborator

sagarpreet-chadha commented Aug 14, 2019 via email

@jywarren
Copy link
Member

OK, i'm looking through this now -- @ananyaarun can you push up your latest changes so I can see the tests you're working with most recently?

jywarren
jywarren reviewed Aug 15, 2019
View reviewed changes
spec/javascripts/indigenousTerritoriesLayers_spec.js Outdated

it("Checks if layer polygons are present", function () {
// There should be atleast one polygon in the Territories layer always .
expect($("#map").children()[0].childNodes[2].childNodes[0].childNodes[0].childNodes.length).toBeGreaterThan(1);
Copy link
Member

Choose a reason for hiding this comment

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

What I might do is to break this into different lines so we can narrow down what is undefined. So

Suggested change
expect($("#map").children()[0].childNodes[2].childNodes[0].childNodes[0].childNodes.length).toBeGreaterThan(1);
expect($("#map")
.children()[0]
.childNodes[2]
.childNodes[0]
.childNodes[0]
.childNodes.length)
.toBeGreaterThan(1);

Maybe? Second, if you look at this example:

https://github.com/jywarren/pxlqst/blob/15887acea6bc297f1acd4785ad4f0ce400abfef4/spec/javascripts/pxlqst_spec.js#L85-L98

It does a pattern like you("walk west toward the torch and arrive there shortly after.", function(done) {

then when the function is supposed to be complete, it calls done(). That allows us to peg completion of the test to something that may not complete immediately.

Here is a pattern for simulating a remote call and running tests on the result:

https://github.com/publiclab/spectral-workbench.js/blob/eaee043e4588716a7b5a0855e5f0d6dbd1de2da1/spec/javascripts/spectrum_spec.js#L332-L367

It may also be helpful!

@ananyaarun
Copy link
Member Author

ananyaarun commented Aug 15, 2019 •
edited
Loading

Hey @jywarren , the latest tests are already pushed.
Also I have pushed the fixture file after adding a polygon object just to test.
The comment below exactly describes the problem we were facing.
Thanks!!

Hey @sagarpreet-chadha @rexagod
I am pretty sure my tests are correct now and I have used the second method for testing.
I have added a polygon abject and I am testing.
There is a problem I am facing 3 days now which I really don't know how to go about.
My tests still fail!!
error
I wrote a very basic test like this just to test and that is failing too I really don't know why.


  it("Checks if layer polygons are present", function () {
  expect($("#map").children()[0].childNodes.length).toBeGreaterThan(-1) ;
  });

PS: Over here i am just checking if it is greater than -1
so that even if for some reason the L.polygon method is not working tests still pass but the problem seems to be with an undefined object now and they still fail.
Map is the div id whose child node we are accessing. Same is done in the PLpeople layer tests.
I really do not understand the undefined object error.
Please let me know if You figure out, Thanks

@sagarpreet-chadha @rexagod @jywarren

@jywarren
Copy link
Member

Also I wanted to apologize for not being available earlier to help with this. It sounds very frustrating. UI testing of remote and asynchronous events is quite complex and I really appreciate you tackling this!

Another thing we can do is this: we can develop a local data source that is stored within the fixtures folder. We can create a test-only map layer that uses the standard layer code, but points at a local resource. Then, we can run tests more simply without requiring any remote data, and it'll be more deterministic and predictable, while still testing all the standardized layer code.

Could this work as well?

@ananyaarun
Copy link
Member Author

ananyaarun commented Aug 15, 2019 •
edited
Loading

No Problem @jywarren
It was a nice challenge to work on this :)

Since the async method kept giving errors, i infact did the method you just suggested above.
Right now in my fixture file i have a layer polygon object defined and loaded. This does not cause any time lag problem on load. But the errors I still get and I really cannot understand why.

I wrote a very basic test like this just to test and that is failing too I really don't know why.

  it("Checks if layer polygons are present", function () {
  expect($("#map").children()[0].childNodes.length).toBeGreaterThan(-1) ;
  });

PS: Over here i am just checking if it is greater than -1
so that even if for some reason the L.polygon method is not working tests should still pass but the problem seems to be with an undefined object now and they still fail.

@jywarren
Copy link
Member

jywarren commented Aug 15, 2019 via email

@ananyaarun
Copy link
Member Author

I think (i may be wrong) if a polygon object is added in the fixture file itself it will not cause any asynchronicity related issues.
What is queer is why the tests fail in a case like my comment above.
I did give the spy method a try but was not able to get that working.

@jywarren
Copy link
Member

jywarren commented Aug 15, 2019 via email

@ananyaarun
Copy link
Member Author

Sure but try to look at this comment @jywarren whenever you have time tomorrow.

Hey @jywarren , the latest tests are already pushed.
Also I have pushed the fixture file after adding a polygon object just to test.
The comment below exactly describes the problem we were facing.
Thanks!!

Hey @sagarpreet-chadha @rexagod
I am pretty sure my tests are correct now and I have used the second method for testing.
I have added a polygon abject and I am testing.
There is a problem I am facing 3 days now which I really don't know how to go about.
My tests still fail!!
error
I wrote a very basic test like this just to test and that is failing too I really don't know why.


  it("Checks if layer polygons are present", function () {
  expect($("#map").children()[0].childNodes.length).toBeGreaterThan(-1) ;
  });

PS: Over here i am just checking if it is greater than -1
so that even if for some reason the L.polygon method is not working tests still pass but the problem seems to be with an undefined object now and they still fail.
Map is the div id whose child node we are accessing. Same is done in the PLpeople layer tests.
I really do not understand the undefined object error.
Please let me know if You figure out, Thanks

@sagarpreet-chadha @rexagod @jywarren

If we can figure this out , I know how to get the tests working with defining a polygon object and that would be a good start to have some tests in place.

@jywarren
Copy link
Member

Hmm. @ananyaarun this is quite frustrating, indeed! What i found was that I can't tell why Testing for PLpeopleLayer passes but your new tests don't -- i tried many things!

However, one clue I did find was this; I added a very simple test:

it("initializes map", function () {
  expect(map).not.toBeUndefined();
});

TO both the People test and to your new tests. It passed in the People test but not in yours!

The second clue; I changed loadFixtures('indigenousLanguagesLayer.html'); to loadFixtures('PLpeopleLayer.html'); in your indigenousLanguagesLayers_spec.js file, and that new initializes map test passed! And in fact it showed:

 Testing for indigenous Language layer
   ✓ Basic Test
   ✓ initializes map
   X Checks if layer polygons are present
     Expected 0 to be greater than 1. (1)

Which is a different error altogether, and more like what you're looking for. So, my theory is that something is wrong with the fixtures. I haven't been able to determine what! But I'd start over with the People fixture and change it bit by bit to see what ultimately causes the initialize map test to fail. I wonder if there is some stray syntax error, or something?

@jywarren
Copy link
Member

AHA!!!!! indigenousLanguagesLayers.html not indigenousLanguagesLayer.html!!!

@jywarren
Copy link
Member

Wheeeee!!!! 🎉

jywarren
jywarren reviewed Aug 16, 2019
View reviewed changes
spec/javascripts/indigenousLanguagesLayers_spec.js Outdated
describe("Testing for indigenous Language layer", function() {

beforeEach(function () {
loadFixtures('indigenousLanguagesLayer.html');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loadFixtures('indigenousLanguagesLayer.html');
loadFixtures('indigenousLanguagesLayers.html');

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh my, I cannot believe this was my error !!
Thanks a lot @jywarren !!!!!!!!

@jywarren
Copy link
Member

I think that's it!!!

@ananyaarun
Copy link
Member Author

Finally this PR is working @sagarpreet-chadha 🎉
I still cannot understand how the initial test passed that time though :/
I guess thats why none of us thought it was such a simple fix.
Anyhow Im so glad it works now !!
Thanks a LOT @jywarren and @sagarpreet-chadha :))

@ananyaarun
Copy link
Member Author

testing

@sagarpreet-chadha
Copy link
Collaborator

AWESOME !!!! @jywarren and @ananyaarun 💯 🎉

sagarpreet-chadha
sagarpreet-chadha approved these changes Aug 18, 2019
View reviewed changes
Copy link
Collaborator

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Superb 🎉

@ananyaarun
Copy link
Member Author

ananyaarun commented Aug 18, 2019 •
edited
Loading

@jywarren Can you do a final review here , and this is good to merge :)

@jywarren
Copy link
Member

It looks awesome. Let's merge and get these tests more universal! Thanks!

@jywarren jywarren merged commit 7533053 into publiclab:master Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren approved these changes

@sagarpreet-chadha sagarpreet-chadha sagarpreet-chadha approved these changes

@rexagod rexagod Awaiting requested review from rexagod

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write tests for the indigenous layers
4 participants
@ananyaarun @sagarpreet-chadha @rexagod @jywarren